cli: Add shell completion generation command #1881
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a bootc completion command to generate shell completion scripts. The implementation is well-intentioned but manually generates the scripts, which is brittle and incomplete. I've provided feedback to use the clap_complete crate for a more robust and maintainable solution, which also aligns the implementation with the existing tests. I've also noted that clap_complete should be a regular dependency, not a dev-dependency, and pointed out some code duplication in the new tests.
| Opt::Completion { shell } => { | ||
| // Build the clap Command from our derived Opt | ||
| let cmd = Opt::command(); | ||
|
|
||
| // Collect visible top-level subcommands and their about text | ||
| fn visible_subcommands(cmd: &clap::Command) -> Vec<(String, String)> { | ||
| let mut subs: Vec<(String, String)> = cmd | ||
| .get_subcommands() | ||
| .filter(|c| { | ||
| // skip hidden subcommands and the help pseudo-command | ||
| if c.is_hide_set() { | ||
| return false; | ||
| } | ||
| if c.get_name() == "help" { | ||
| return false; | ||
| } | ||
| true | ||
| }) | ||
| .map(|c| { | ||
| let name = c.get_name().to_string(); | ||
| let about = c.get_about().map(|s| s.to_string()).unwrap_or_default(); | ||
| (name, about) | ||
| }) | ||
| .collect(); | ||
| subs.sort_by_key(|(n, _)| n.clone()); | ||
| subs | ||
| } | ||
|
|
||
| let subs = visible_subcommands(&cmd); | ||
|
|
||
| match shell { | ||
| CompletionShell::Zsh => { | ||
| // zsh: produce a simple _describe-based completion with descriptions | ||
| println!("#compdef bootc"); | ||
| println!("# Generated by bootc"); | ||
| println!("_bootc() {{"); | ||
| println!(" local -a commands"); | ||
| print!(" commands=("); | ||
| for (name, about) in &subs { | ||
| // escape single quotes | ||
| let about_esc = about.replace('\'', "'\\''"); | ||
| print!(" '{}:{}'", name, about_esc); | ||
| } | ||
| println!(" )"); | ||
| println!(" _describe 'bootc commands' commands"); | ||
| println!("}}"); | ||
| println!("compdef _bootc bootc"); | ||
| } | ||
| CompletionShell::Fish => { | ||
| // fish: emit a complete line per command with description | ||
| println!("# Generated by bootc"); | ||
| for (name, about) in &subs { | ||
| let about_esc = about.replace('"', "\\\""); | ||
| println!("complete -c bootc -n '__fish_use_subcommand' -a '{}' -d \"{}\"", name, about_esc); | ||
| } | ||
| } | ||
| CompletionShell::Bash => { | ||
| // bash: generate a simple completer that only lists top-level subcommands | ||
| println!("# Generated by bootc"); | ||
| println!("_bootc() {{"); | ||
| println!(" local cur prev words cword"); | ||
| println!(" _init_completion || return"); | ||
| print!(" local -a cmds=("); | ||
| for (name, _about) in &subs { | ||
| print!(" {}", name); | ||
| } | ||
| println!(" )"); | ||
| println!(" if [ $COMP_CWORD -eq 1 ]; then"); | ||
| println!(" COMPREPLY=( $(compgen -W \"${{cmds[*]}}\" -- \"$cur\") )"); | ||
| println!(" return 0"); | ||
| println!(" fi"); | ||
| println!("}}"); | ||
| println!("complete -F _bootc bootc"); | ||
| } | ||
| }; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The current implementation of shell completion script generation is done manually. This is brittle, incomplete (only supports top-level subcommands), and hard to maintain. The clap_complete crate, which is already added as a dependency, provides a robust way to generate comprehensive completion scripts. Using it would be more idiomatic and ensure that completions for all subcommands, options, and arguments are generated correctly.
Furthermore, the test test_generate_completion_scripts_contain_commands already uses clap_complete::generate, which means the test is not testing the actual implementation of the completion command. Aligning the implementation with the test will fix this discrepancy.
I suggest replacing the manual generation with clap_complete::generate.
Opt::Completion { shell } => {
use clap_complete::{generate, shells};
let mut cmd = Opt::command();
let mut stdout = std::io::stdout();
let bin_name = "bootc";
match shell {
CompletionShell::Bash => generate(shells::Bash, &mut cmd, bin_name, &mut stdout),
CompletionShell::Zsh => generate(shells::Zsh, &mut cmd, bin_name, &mut stdout),
CompletionShell::Fish => generate(shells::Fish, &mut cmd, bin_name, &mut stdout),
};
Ok(())
}
crates/lib/Cargo.toml
Outdated
| uapi-version = "0.4.0" | ||
|
|
||
| [dev-dependencies] | ||
| clap_complete = "4" |
There was a problem hiding this comment.
clap_complete is used in the bootc completion command, which is part of the main binary. Therefore, it should be a regular dependency, not a dev-dependency. Please move clap_complete = "4" from [dev-dependencies] to [dependencies]. This will also allow you to use clap_complete::generate in the implementation of the completion command, making it more robust and consistent with the tests.
| fn visible_subcommands_filter_and_sort() { | ||
| let cmd = Opt::command(); | ||
| // use the same helper as completion | ||
| let subs = { | ||
| fn visible_subcommands_for_test(cmd: &clap::Command) -> Vec<String> { | ||
| let mut names: Vec<String> = cmd | ||
| .get_subcommands() | ||
| .filter(|c| { | ||
| if c.is_hide_set() { | ||
| return false; | ||
| } | ||
| if c.get_name() == "help" { | ||
| return false; | ||
| } | ||
| true | ||
| }) | ||
| .map(|c| c.get_name().to_string()) | ||
| .collect(); | ||
| names.sort(); | ||
| names | ||
| } | ||
| visible_subcommands_for_test(&cmd) | ||
| }; | ||
|
|
||
| // basic expectations: completion subcommand is hidden and must not appear | ||
| assert!(!subs.iter().any(|s| s == "completion")); | ||
| // help must not be present | ||
| assert!(!subs.iter().any(|s| s == "help")); | ||
| // ensure sorted order | ||
| let mut sorted = subs.clone(); | ||
| sorted.sort(); | ||
| assert_eq!(subs, sorted); | ||
| } |
There was a problem hiding this comment.
This test contains a helper function visible_subcommands_for_test which duplicates the logic of visible_subcommands from the run_from_opt function. Duplicating code like this makes maintenance harder, as changes need to be applied in two places.
If you adopt the suggestion to use clap_complete for generating completions, both visible_subcommands and this test will become obsolete and can be removed, which would be the ideal solution.
If you decide to keep the manual implementation, consider extracting visible_subcommands into a standalone function (outside of run_from_opt) so it can be shared between the command implementation and this test, removing the code duplication.
Add a hidden 'bootc completion <shell>' subcommand that generates shell completion scripts for bash, zsh, and fish. This enables distributions to generate and package shell completions during RPM/deb build by calling 'bootc completion bash' etc. The completion scripts include descriptions for all visible subcommands and support prefix filtering for a better user experience. Signed-off-by: Shion Tanaka <shtanaka@redhat.com>
tnk4on
force-pushed
the
feature/shell-completion
branch
from
4676f0f to
1c81141
Compare
1 participant
Summary
Add a hidden
bootc completion <shell>subcommand that generates shell completion scripts for bash, zsh, and fish.Motivation
This enables distributions (e.g., Fedora, CentOS Stream, RHEL) to generate and package shell completions during RPM/deb builds by calling:
Implementation Details
CompletionShellenum with Bash, Zsh, and Fish variantsclap::CommandFactoryto introspect the CLI structureclap_completeas a dev-dependencyTesting