Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make --socket-path pull ETHERSYNC_SOCKET from env via clap #127

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

cosmicexplorer
Copy link
Contributor

Problem

It's much easier to set ETHERSYNC_SOCKET via the environment for many use cases instead of modifying the command line, as noted by @zormit in #112 (comment). We currently do this by hand in the nvim plugin:

local socket_path = os.getenv("ETHERSYNC_SOCKET")
if socket_path then
table.insert(params, "--socket-path=" .. socket_path)
end

We could do the same for the emacs plugin in #112, but it might be nice for the ethersync binary to support this itself.

Solution

  • Add "env" feature to our clap dependency.
  • Set Arg::env() for the global socket_path: PathBuf argument.
    • A command-line --socket-path argument overrides this value; the env var is only read if the argument is not provided.
    • Note that the daemon is now also affected by ETHERSYNC_SOCKET in addition to the client.
  • Remove code in nvim plugin to translate the ETHERSYNC_SOCKET env var into --socket-path.

Result

All editor plugins (and the fuzzer) can expect to be able to set the ETHERSYNC_SOCKET environment variable as a separate way to parameterize the ethersync client, and users can set ETHERSYNC_SOCKET in their environment when invoking the daemon.

@cosmicexplorer cosmicexplorer mentioned this pull request Aug 14, 2024
Copy link
Contributor

@blinry blinry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thoughtful PR! Very cool that clap has a built-in feature for this!

daemon/src/main.rs Show resolved Hide resolved
daemon/src/main.rs Outdated Show resolved Hide resolved
vim-plugin/plugin/ethersync.lua Show resolved Hide resolved
daemon/src/actors.rs Outdated Show resolved Hide resolved
daemon/src/bin/fuzzer.rs Outdated Show resolved Hide resolved
daemon/src/bin/fuzzer.rs Outdated Show resolved Hide resolved
daemon/src/bin/fuzzer.rs Outdated Show resolved Hide resolved
blinry
blinry previously requested changes Aug 14, 2024
Copy link
Contributor

@blinry blinry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems really good to me! ✨

The last thing I'd request is undoing the mode change on fuzzer.rs spotted by @zormit :)

After that, it'd be great if you could squash the small "fixup" commits into the others, and then we can merge this! 🎉

- remove ETHERSYNC_SOCKET conversion from nvim plugin
- add info log for socket path if set from env in daemon
@zormit zormit requested a review from blinry August 14, 2024 20:45
@zormit zormit dismissed blinry’s stale review August 14, 2024 20:58

I want to merge it now and "Changes requested" is blocking this :)

@zormit zormit removed the request for review from blinry August 14, 2024 20:59
@zormit zormit merged commit 9413027 into ethersync:main Aug 14, 2024
3 checks passed
@zormit
Copy link
Contributor

zormit commented Aug 14, 2024

Thanks for this, @cosmicexplorer! =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants