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.

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

Conversation

pseyfert
Copy link
Contributor

@pseyfert pseyfert commented May 9, 2022

Iiuc the value hint CommandWithArgs for zsh was incorrect when in a positional argument after the first positional argument.

I updated the clap_complete/tests/common.rs:value_hint_command accordingly to generate a test case. The relevant part is

clap::Command::new(name)
    .trailing_var_arg(true)
    .arg(clap::Arg::new("positional")
        .required(true)
        .possible_values(["stable", "nightly"]),
    )
    .arg(clap::Arg::new("command_with_arg")
        .takes_value(true)
        .multiple_values(true)
        .value_hint(clap::ValueHint::CommandWithArguments),
    )

The resulting section of the completion function is then without my change to zsh.rs

_arguments \
':positional:(stable nightly)' \
'*::command_with_args:_cmdambivalent'

With this function, I don't get any completion past my-app stable TAB. From the zshcompsys man page https://github.com/zsh-users/zsh/blob/master/Doc/Zsh/compsys.yo#L3903= (section on _arguments for the forms *:message:action with one, two, or three colons):

 *:message:action
 *::message:action
 *:::message:action
        This  describes  how  arguments (usually non-option
        arguments, those not beginning with - or +) are  to
        be  completed  when  neither of the first two forms
        was provided.  Any number of arguments can be  com‐
        pleted in this fashion.

        With  two colons before the message, the words spe‐
        cial array and the CURRENT  special  parameter  are
        modified to refer only to the normal arguments when
        the action is executed or  evaluated.   With  three
        colons  before the message they are modified to re‐
        fer only to the normal arguments  covered  by  this
        description.

I.e. the completion now tries to complete a command stable ….

With my change, the third colon gets added, such that _cmdambivalent only "sees" the arguments after the stable/nightly argument:

_arguments \
':positional:(stable nightly)' \
'*:::command_with_args:_cmdambivalent'

As cross check, I tried my-app stable TAB and got tons of suggestions, all seeming like valid commands I have installed. To try further I ran my-app stable git cl<TAB> and got clean and clone suggested.

NB 1: I only focussed on this one test case. The other changes to the test. I haven't looked long at the changes to my-app help TAB I'm questioning here if my-app help TAB should complete more than one subcommand? This is to me less a question about what the desired zsh completion code is but more about this opt in to multiple occurences

NB 2: Obviously my change alters the snapshots for other shells, I blindly updated the snapshot here.

@pseyfert pseyfert force-pushed the fixup/cmd branch 2 times, most recently from 593672d to a5ba355 Compare May 9, 2022 19:19
This fixes e.g. CommandWithArguments with previous positional arguments
in zsh.
@pseyfert pseyfert changed the title Fix CommandWithArguments with previous positional args fix(complete): CommandWithArguments with previous positional args in zsh May 10, 2022
Comment on lines 635 to +636
let cardinality = if arg.is_multiple_values_set() || arg.is_multiple_occurrences_set() {
"*:"
"*::"
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.

@epage
Copy link
Member

epage commented Jan 3, 2023

As this hasn't got much attention, I'm going to go ahead and close this out. If someone wants to pick this back, up, I'd recommend focusing on our clap-native completion support.

@epage
Copy link
Member

epage commented Jul 21, 2023

btw if someone picks this up, we can now do testing using #5026

@epage epage closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants