feat: Add shell completion generation#326
Conversation
792d3ff to
02d2ac4
Compare
|
Thanks for this. I haven't had a chance to review it yet, but this should be a nice QOL improvement.
We don't even support windows, so not right now. |
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, ValueEnum)] | ||
| pub enum CompletionShell { |
There was a problem hiding this comment.
Let's mark this with #[non_exhaustive] so that it isn't a breaking change to add a new enum variant in the future.
| match shell { | ||
| clap_complete::aot::Shell::Bash => { | ||
| let script = String::from_utf8(generated) | ||
| .context("generated bash completion was not valid UTF-8")?; |
There was a problem hiding this comment.
Context strings get stacked up below the main root cause error, so they should have a form that will read well in that context. It looks awkward when there is a root cause error that sounds like a specific error and some of the context strings also sound like a specific root cause error, so basically context strings should read like "while doing x."
In this case "parsing bash completions as utf8" would be a good option.
You followed that context pattern nicely below for the write_all error.
| if let Some(parent_dir) = file_path.parent() { | ||
| if !parent_dir.as_os_str().is_empty() { | ||
| fs::create_dir_all(parent_dir).with_context(|| { | ||
| format!("Failed to create directory: {}", parent_dir.display()) |
There was a problem hiding this comment.
same context style nit as above
| } | ||
| } | ||
| let mut file = fs::File::create(file_path) | ||
| .with_context(|| format!("Failed to create file: {}", file_path.display()))?; |
There was a problem hiding this comment.
same context style nit as above
| word="${{COMP_WORDS[$index]}}" | ||
| case "$word" in | ||
| # Keep in sync with the Commands enum in libshpool/src/lib.rs. | ||
| version|daemon|attach|detach|kill|list|completion|set-log-level|help) |
There was a problem hiding this comment.
It looks like we may be able to auto-generate this list and avoid having to keep it in sync by deriving ValueEnum and then using the value_variants method. The types are a bit weird since it returns a slice of Self, but I think if we invoke value_variants on the top level type, that will given an iterator over the subcommands and then we can downcast to get something with a string with the to_possible_value thing.
| if !args.no_daemonize | ||
| && !matches!( | ||
| args.command, | ||
| Commands::Version | Commands::Daemon | Commands::Completion { .. } |
There was a problem hiding this comment.
I don't think we need Commands::Completion here because it is handled on line 657. The filter for version is nice though.
That shouldn't ever be matched if the user of libshpool is doing it right because the binary is supposed to handle the version case, but it is nice defensive coding.
| /// automatic version support for a library. | ||
| #[derive(Parser, Debug, Default)] | ||
| #[clap(author, about)] | ||
| #[clap(author, about, name = "shpool")] |
There was a problem hiding this comment.
Since this is a library and users can wrap it with their own main we shouldn't be too prescriptive about the name used in completion generation or helptext. Let's leave this off so that clap will automatically pull the binary name from argv[0]
|
I left an initial round of feedback. To be honest, I've never messed around with manually writing completion scripts, so it's rather hard for me to review the custom augmentation code. I'll need to find a time when I can sit down and learn about how that works, but that could take me a while. If you want, you could split this PR up into two PRs, one that just does the barebones clap integration and one that adds in the agumentation, and we should be able to get the barebones integration one merged pretty quickly. It's also totally fine to leave it as one PR and just wait until I have the cycles for an in-depth review. |
There is an incredible large Powershell community on Linux... it's not Windows only. |
Thanks
My first idea was to auto-generate it. But we have a lot of dynamic components that check which shells are currently active - and those are exactly the ones that need to be generated in the autocomplete section. So I looked at how other projects handle this. I’m not happy with it either, but unfortunately I don’t have any ideas or experience on how to improve it.
Okay, I'll give it a go, but need to finish another project first. |
It's not so much that I'm not happy with it, just that I'm unfamiliar with it so I need more time than usual to review the code. |
This is just a first try, which I had tumbling on my disk for some weeks... I updated it to latest discussions.
Summary
shpool completion <shell>command backed byclap_complete; supported targets are bash, elvish, fish, and zsh.-o, --output.Testing
cargo fmt --allcargo test -p libshpool completion_testscargo build --releaseRisks and mitigations
clap_completeoutput; the code now fails fast if the expected generated function signature changes.shpool listat completion time for Bash and Fish; this is limited to local CLI usage and forwards the selected global socket/config flags so the lookup stays scoped to the active daemon.Discussions
References