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.

Sign up for GitHub

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

fix(complete): CommandWithArguments with previous positional args in zsh #3710

Closed
wants to merge 1 commit into from
Closed

fix(complete): CommandWithArguments with previous positional args in zsh #3710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clap_complete/src/shells/zsh.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ fn write_positionals_of(p: &Command) -> String {
debug!("write_positionals_of:iter: arg={}", arg.get_id());

let cardinality = if arg.is_multiple_values_set() || arg.is_multiple_occurrences_set() {
"*:"
"*::"
Comment on lines 635 to +636
Copy link
Member

Choose a reason for hiding this comment

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

Reading through the docs, I'm not seeing anything that gives me confidence one way or the other that adding an extra colon for all cases is the correct way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking.

I was thinking a completion for trailing arguments usually doesn't need to know a previous positional argument (each argument having its own PossibleValues or ValueHint.). But given your concern I tried to come up with a counter example (not tested, but i think we can construct an app that my change breaks from this draft):

Command::new("explain_basic_cli_tools").arg(Arg::new("program").possible_value(["git", "cargo"]).required(true)).arg(Arg::new("args").multiple_values(true).value_hint(CommandWithArguments))

where we want users to call explain_basic_cli_tools cargo example … or explain_basic_cli_tools git worktree add … and artificially restrict possible commands.

For other value hints I may also introduce a regression if we have a file completer for a positional argument and the trailing arguments. then zsh's behaviour to let a file appear multiple times may get altered (didn't check, but i agree with you, from the generic point of view here in clap it's hard to say that either three colons or one/two colons is in general right).

fwiw I ended up in this rabbit hole looking at the completion of rustup run nightly cargo bui<TAB> where the colon is desired, otherwise we end up completing for a command nightly instead of cargo. too bad, i though this part was easy 😩

you don't happen to see a way out of this?

Copy link
Member

Choose a reason for hiding this comment

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

I barely know anything about completions which makes reviewing them frustrating :)

We are working on #3166 which should remove the complications of dealing with esoteric syntax in specific cases. For external subcommands, we will probably have to provide a way for people to provide their own shell function much like we should add for our static completions today.

} else if !arg.is_required_set() {
":"
} else {
Expand Down
5 changes: 5 additions & 0 deletions clap_complete/tests/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,11 @@ pub fn value_hint_command(name: &'static str) -> clap::Command<'static> {
.short('c')
.value_hint(clap::ValueHint::CommandString),
)
.arg(
clap::Arg::new("positional")
.required(true)
.possible_values(["stable", "nightly"]),
)
.arg(
clap::Arg::new("command_with_args")
.takes_value(true)
Expand Down
2 changes: 1 addition & 1 deletion clap_complete/tests/snapshots/basic.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ _arguments "${_arguments_options[@]}" /
(help)
_arguments "${_arguments_options[@]}" /
'-c[]' /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand Down
2 changes: 1 addition & 1 deletion clap_complete/tests/snapshots/feature_sample.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ _arguments "${_arguments_options[@]}" /
;;
(help)
_arguments "${_arguments_options[@]}" /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand Down
2 changes: 1 addition & 1 deletion clap_complete/tests/snapshots/quoting.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ _arguments "${_arguments_options[@]}" /
;;
(help)
_arguments "${_arguments_options[@]}" /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand Down
4 changes: 2 additions & 2 deletions clap_complete/tests/snapshots/special_commands.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ _arguments "${_arguments_options[@]}" /
'--help[Print help information]' /
'-V[Print version information]' /
'--version[Print version information]' /
'*::path:' /
'*:::path:' /
&& ret=0
;;
(some-cmd-with-hyphens)
Expand All @@ -71,7 +71,7 @@ _arguments "${_arguments_options[@]}" /
;;
(help)
_arguments "${_arguments_options[@]}" /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand Down
4 changes: 2 additions & 2 deletions clap_complete/tests/snapshots/sub_subcommands.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ _arguments "${_arguments_options[@]}" /
;;
(help)
_arguments "${_arguments_options[@]}" /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand All @@ -79,7 +79,7 @@ esac
;;
(help)
_arguments "${_arguments_options[@]}" /
'*::subcommand -- The subcommand whose help message to display:' /
'*:::subcommand -- The subcommand whose help message to display:' /
&& ret=0
;;
esac
Expand Down
2 changes: 1 addition & 1 deletion clap_complete/tests/snapshots/value_hint.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ _my-app() {

case "${cmd}" in
my__app)
opts="-p -f -d -e -c -u -h --help --choice --unknown --other --path --file --dir --exe --cmd-name --cmd --user --host --url --email <command_with_args>..."
opts="-p -f -d -e -c -u -h --help --choice --unknown --other --path --file --dir --exe --cmd-name --cmd --user --host --url --email stable nightly <command_with_args>..."
if [[ ${cur} == -* || ${COMP_CWORD} -eq 1 ]] ; then
COMPREPLY=( $(compgen -W "${opts}" -- "${cur}") )
return 0
Expand Down
3 changes: 2 additions & 1 deletion clap_complete/tests/snapshots/value_hint.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ _my-app() {
'--url=[]: :_urls' /
'--email=[]: :_email_addresses' /
'--help[Print help information]' /
'*::command_with_args:_cmdambivalent' /
':positional:(stable nightly)' /
'*:::command_with_args:_cmdambivalent' /
&& ret=0
}

Expand Down