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

Jump to bottom

clap_complete: fix support for two multi-length arguments in zsh #4612

Merged
merged 1 commit into from
Feb 23, 2023
Merged

clap_complete: fix support for two multi-length arguments in zsh #4612

merged 1 commit into from
Feb 23, 2023

Conversation

davvid
Copy link
Contributor

@davvid davvid commented Jan 9, 2023 •
edited
Loading

Properly handle two consecutive multi-valued arguments.

Currently the zsh completion generates an error:

$ my-app <TAB>
_arguments:comparguments:325: doubled rest argument definition:
*::second -- second set of of multi-length arguments:

Special-case multi-valued arguments to avoid emitting duplicate catch-all arguments.

Closes #3022
Signed-off-by: David Aguilar davvid@gmail.com

This was referenced Jan 9, 2023
Closed
Closed
@davvid davvid force-pushed the zsh-multi-length-arguments branch 6 times, most recently from b8fab94 to a686e61 Compare January 9, 2023 11:25
epage
epage reviewed Jan 9, 2023
View reviewed changes
src/builder/arg.rs Outdated Show resolved Hide resolved
clap_complete/src/shells/zsh.rs Show resolved Hide resolved
clap_complete/tests/common.rs Outdated Show resolved Hide resolved
clap_complete/src/shells/zsh.rs Outdated Show resolved Hide resolved
@davvid davvid force-pushed the zsh-multi-length-arguments branch 6 times, most recently from 448865a to 3123e33 Compare January 10, 2023 06:23
@davvid
Copy link
Contributor Author

davvid commented Jan 10, 2023

I rebased a bunch of times and that may have caused this note to have gotten lost:

You can have two Vec args separated with a value_terminator
You can have two Vec args with the second one set to last (raw sets last)
which will force a separator of -- before the second

I also saw a note about using ; as the separator in the test because -- is a special value. That makes a lot of sense.

I'm going to keep the single-arg test and add an additional test with two args. I'm also going to adjust the logic so that the penultimate argument is special-cased. I'm going to make it the penultimate multi-value argument automatically inject a value terminator, either user-defined or the special default -- terminator, when the final argument is also multi-valued.

I think that should handle the edge cases around two multi-valued arguments. Existing code should pickup this fix without needing to add any annotations, etc.

Thanks again for your reviews. I'll be pushing final updates into this PR shortly.

@davvid davvid force-pushed the zsh-multi-length-arguments branch 2 times, most recently from f56b2c1 to 9ce49bb Compare January 10, 2023 09:06
epage
epage reviewed Jan 10, 2023
View reviewed changes
Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

The first two commits seem ready but I feel there is more to work out in the last. If you are wanting the first fix earlier, we can split this into two PRs and merge the value_terminator work now.

clap_complete/src/shells/zsh.rs Outdated Show resolved Hide resolved
clap_complete/src/shells/zsh.rs Outdated Show resolved Hide resolved
clap_complete/src/shells/zsh.rs Outdated Show resolved Hide resolved
@davvid davvid mentioned this pull request Jan 10, 2023
Merged
@davvid
Copy link
Contributor Author

davvid commented Jan 10, 2023

Thanks for the sug to split this PR in two. The initial two commits are now in #4624 while we figure out what to do with the more complicated edge cases.

@davvid
fix(complete): Handle multi-valued arguments
170bd59 
zsh completions for commands that have multiple Vec arguments require
special care.

We can have two Vec args separated with a value terminator.
We can also have two Vec args with no value terminators specified
where the final arg uses 'raw' and thus requires '--' to be used.

The 2nd of these scenarios requires special handling to avoid
emitting a duplicate '*:arguments' completion entry.

Currently, the zsh completions generate an error in this scenario:

    $ my-app <TAB>
    _arguments:...: doubled rest argument definition:
    *::second -- second set of of multi-length arguments:

We already use the '-S' option when calling _arguments.
This option makes it so that completion stops after '--' is encountered.
This means that the handling for trailing 'raw' arguments does not need
to specified.

Special-case multi-valued arguments so that we can skip emitting
the final multi-valued argument if a previous multi-valued argument
has already been emitted.

Closes clap-rs#3022
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to garden-rs/garden that referenced this pull request Jan 21, 2023
@davvid
completion: add value_terminator to workaround clap-rs/clap#3022
e6a471c 
Now that clap-rs/clap#4624 was merged we can use value_terminator to fix the
zsh completions for commands that use two consecutive multi-valued arguments.

A more complete solution may arrive in clap-rs/clap#4612 but until that is
merged we can use this solution for now. If that change is ever merged then
we can bump our clap_complete version and remove our use of value_terminator.

Related-to: #10
Related-to: clap-rs/clap#3022
Related-to: clap-rs/clap#4624
Signed-off-by: David Aguilar <davvid@gmail.com>
davvid added a commit to garden-rs/garden that referenced this pull request Jan 21, 2023
@davvid
doc: add a link to clap-rs/clap#4612 for reference
18a857f 
Related-to: #10
Signed-off-by: David Aguilar <davvid@gmail.com>
@chris-olszewski chris-olszewski mentioned this pull request Feb 14, 2023
Closed
@skovhus
Copy link

skovhus commented Feb 23, 2023

Great work here. How close is this to completion?

@epage
Copy link
Member

epage commented Feb 23, 2023

Sorry, I had gotten busy with another project and I'm now getting a chance to go through my backlog. I suspect there are still corner cases but this is a case where some improvement is better than no improvement, so I'll go ahead and merge.

@epage epage merged commit 7780333 into clap-rs:master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clap_generate: zsh broken with two multi length arguments
3 participants
@davvid @skovhus @epage