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

Don't recommend saving shell completions to disk #782

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

wfchandler
Copy link
Contributor

Currently our docs advise users of several shells to save the output of oxide completion to a file and source that in their init. This has led to users having outdated completions when they upgrade to a new version of the CLI.

Update the docs to recommend generating the completions on the fly at startup. Doing this will add a bit of latency to their shell startup, but user instructions should prioritize reliability over performance.

@wfchandler wfchandler requested a review from ahl August 8, 2024 17:01
@wfchandler
Copy link
Contributor Author

Validated updated instructions with zsh, fish, and elvish locally.

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

I'm for it.

image

Currently our docs advise users of several shells to save the output of
`oxide completion` to a file and source that in their init. This has led
to users having outdated completions when they upgrade to a new version
of the CLI.

Update the docs to recommend generating the completions on the fly at
startup. Doing this will add a bit of latency to their shell startup,
but user instructions should prioritize reliability over performance.
@wfchandler
Copy link
Contributor Author

@david-crespo it's a bit slower than that, since the shell needs to evaluate the generated text:

$ hyperfine  'zsh -c eval "$(oxide completion -s zsh)"'
Benchmark 1: zsh -c eval "$(oxide completion -s zsh)"
  Time (mean ± σ):      32.7 ms ±   0.7 ms    [User: 12.5 ms, System: 5.0 ms]
  Range (min … max):    31.6 ms …  35.4 ms    85 runs

@david-crespo
Copy link
Contributor

Eagerly awaiting the M4 MBPs

image

@@ -27,26 +27,20 @@ use std::io;
///
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't get why this isn't working for me on macos:

$ eval $(./target/debug/oxide completion --shell bash)
-bash: syntax error near unexpected token `)'
$ ./target/debug/oxide completion --shell bash > foo
$ . foo
# works fine

Copy link
Contributor

@david-crespo david-crespo Aug 12, 2024

Choose a reason for hiding this comment

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

try quotes around the $()?

Edit: yep this makes it not error out

Copy link
Contributor

Choose a reason for hiding this comment

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

also the default shell on mac is zsh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed that error is due to lack of quotes.

bash-5.2$ eval $(./target/debug/oxide completion -s bash)
bash: syntax error near unexpected token `)'
bash-5.2$ eval "$(./target/debug/oxide completion -s bash)"
bash-5.2$ echo $?
0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! my bad.

/// ```
///
/// and check that you have the following lines in your `~/.zshrc`:
/// Add this to your `~/.zshrc`:
///
/// ```sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

these ```sh are in the console output for oxide completion --help -- we don't have to change that now, but that seems kind of lousy to me -- what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted to be really cool we could render the markdown

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wfchandler wfchandler merged commit 9b01848 into main Aug 12, 2024
16 checks passed
@wfchandler wfchandler deleted the wc/update-completion-doc branch August 12, 2024 18:54
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.

4 participants