-
Notifications
You must be signed in to change notification settings - Fork 49
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
[PM-3029] Add support for shell completion #103
Conversation
No New Or Fixed Issues Found |
Do we know what the convention is for shell completion and manpages for rust packages? Feels somewhat odd to have the CLI itself generate them. |
I don't think there are specific conventions for rust packages, usually man pages are included in the system install packages (so .deb or .rpm in Linux), and it's the system package manager that moves them to the correct place. We could generate them at build time and include them in the release zip, but without an automatic installation step that might end up confusing users. Shell autocompletion on the other hand I've noticed a lot of software handle the generation from the CLI, I assume because they can't control which shell the user is running, and the generated autocomplete code is different for all of them, for example: |
Please split this into two PRs. We'll try and get shell completion merged while thinking of what we should do about man pages. |
# Conflicts: # crates/bws/Cargo.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this to the readme?
## How to enable shell autocompletions | ||
|
||
### Zsh | ||
|
||
If completion is not enabled already, you need to enable it first: | ||
|
||
```zsh | ||
echo "autoload -U compinit; compinit" >> ~/.zshrc | ||
``` | ||
|
||
Enable autocompletions for the current user: | ||
|
||
```zsh | ||
echo 'source <(/path/to/bws completions zsh)' >> ~/.zshrc | ||
``` | ||
|
||
### Bash | ||
|
||
Enable autocompletions for the current user: | ||
|
||
```zsh | ||
echo 'source <(/path/to/bws completions bash)' >> ~/.bashrc | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in the readme or the help docs? The readme is only really visible on crates.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help docs should probably only be updated after we make a release with these changes merged in I think, otherwise it might be confusing for users of the existing release.
crates/bws/src/main.rs
Outdated
if let Commands::Completions { shell } = command { | ||
let Some(shell) = shell.or_else(Shell::from_env) else { | ||
eprintln!("Couldn't autodetect a valid shell. Run `bws completions --help` for more info."); | ||
std::process::exit(1); | ||
}; | ||
|
||
let mut cmd = Cli::command(); | ||
let name = cmd.get_name().to_string(); | ||
clap_complete::generate(shell, &mut cmd, name, &mut std::io::stdout()); | ||
return Ok(()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're starting to have a few if let here, should we try and use a switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, we should probably consider extracting them to a separate function in the future because the process_commands is already getting pretty big, but that's a bigger refactoring.
## Type of change ``` - [ ] Bug fix - [x] New feature development - [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc) - [ ] Build/deploy pipeline (DevOps) - [ ] Other ``` ## Objective Implement support for generating manpages at compile-time. Extracted from PR #103. The manpages also get generated in CI and uploaded as artifacts. Note that because the CLI struct needs to be accessed by a build script, it had to be extracted to a separate self contained file. ## How to generate manpages locally The manpages get generated by a build script, and are available in the crate's build script OUT_DIR, to get it: ``` cargo build --message-format json > build.json jq -r --slurp '.[] | select (.reason == "build-script-executed") | select(.package_id|contains("crates/bws")) .out_dir' build.json ``` The output path is going to be something like `sdk/target/debug/build/bws-4acb75a675879df1/out`. Copy them to a better location, and then you can view the pages: - `man ./my-pages/bws.1` - `man ./my-pages/bws-project.1` - `man ./my-pages/bws-secret.1` - … If you install them in a system path, you could also just access them with `man bws`, but that path is system specific. In Ubuntu I think it is `/usr/share/man` ## Screenshots ![Screenshot 2023-07-14 at 18 28 54](https://github.com/bitwarden/sdk/assets/725423/549fbaf8-1d1a-4d77-b348-61e0ddba6911) ![image](https://github.com/bitwarden/sdk/assets/725423/6b0725d7-f307-42db-aa24-88aff7245e3b)
Type of change
Objective
Add support for shell completion using the official crates from the Clap project.
I've made a command for each to be generated at runtime, but we could potentially also generate them at build time, though that will require to move the
Cli
andCommands
structs to a separate file. Build time generation might make more sense if we ever want to distribute.deb
or.rpm
packages.Code changes
#[command(name)]
tobws
to represent the binary name, used by the shell completions.Completions
: Receives a single parameter for the type of shell to generate the completions for (Bash, zsh, pwsh, etc), if not provided it will try autodetecting the current shell.Note that both these changes seem to ignore the
hide
parameter set in the deprecated commands, so we might want to consider holding off merging this until they are removed.How to enable completions
This varies by shell, in
zsh
:Edit the file
~/.zshrc
and append to the end the following line:Screenshots
Screen.Recording.2023-07-14.at.18.27.32.mov