-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(cli): Add support for completing --config argument values with native-completions
#16249
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
base: master
Are you sure you want to change the base?
Conversation
Add value_hint for the global --config arg to allow completing its value. Signed-off-by: Nick Spinale <[email protected]>
| .global(true) | ||
| .hide(true)) | ||
| .arg(multi_opt("config", "KEY=VALUE|PATH", "Override a configuration value").global(true)) | ||
| .arg(multi_opt("config", "KEY=VALUE|PATH", "Override a configuration value").value_hint(clap::ValueHint::FilePath).global(true)) |
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.
Maybe one thing worth discussion. According to the doc:
Note: To maintain consistency with existing .cargo/config.toml probing behavior, it is by design that a path in a config file passed via --config is also relative to two levels up from the config file itself.
So personally would expect people usually put their extra config under .cargo/ or .config/. Not sure if this means that we might want to have custom completer
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.
If we filter for these, they'll be shown first but all other dirs will still be shown but after.
Whats more of a problem is that it hides the candidates of hidden files and directories. In that state, they are only shown if there are no visible candidates.
I might want to add an override for this in clap. It would be easy to say "don't hide for this completer" but that will then show everything when likely only a subset is wanted. I could have a separate hide function that takes a predicate to override the default hide behavior.
| .global(true) | ||
| .hide(true)) | ||
| .arg(multi_opt("config", "KEY=VALUE|PATH", "Override a configuration value").global(true)) | ||
| .arg(multi_opt("config", "KEY=VALUE|PATH", "Override a configuration value").value_hint(clap::ValueHint::FilePath).global(true)) |
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.
A potential step up is to limit completions to *.toml with https://docs.rs/clap_complete/latest/clap_complete/engine/struct.PathCompleter.html#method.filter
Add
value_hintfor the global--configargument to allow completing its value.The related #16245 (comment) adds partial support to
src/etc/cargo.bashcomp.sh.What does this PR try to resolve?
Currently,
native-completionssupports completing the--configflag itself, but not its argument value. This PR adds support for completing its value with a file path (clap::ValueHint::FilePath).How to test and review this PR?
In a shell supported by
native-completions, run:Then, observe that the following completion behaves as expected. That is, the user is prompted with files paths.