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: Resolve conflicting name inference if from aliases #5025

Merged
merged 3 commits into from
Sep 25, 2023
Merged

fix: Resolve conflicting name inference if from aliases #5025

merged 3 commits into from
Sep 25, 2023

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Jul 19, 2023

This fixes 3 behaviors with inferring subcommands

  • When name matches are for the same command, allow the match
  • Don't panic when inferring long_flags because of a mix up between flag and name
  • When long flag matches are for the same command, allow the match

Testing of long flags was covered in #4971

Fixes #5021

@epage
Copy link
Member

epage commented Jul 19, 2023

Did you verify there are tests for the two other infer cases?

@SUPERCILEX
Copy link
Contributor Author

Yeah: conflict test, simple, and exact.

@epage
Copy link
Member

epage commented Jul 19, 2023

To be more precise, in #5021, I asked for a fix to the issue to also verify that longs and subcommand longs have tests to verify they don't conflict with their aliases when doing inferring.

@SUPERCILEX
Copy link
Contributor Author

What do you mean by "longs and subcommand longs"? If longs means --foo, then tests were added in the last PR. And if subcommand longs means ./exe sub, then this PR adds a test for alias conflicts.

@epage
Copy link
Member

epage commented Jul 20, 2023

If longs means --foo, then tests were added in the last PR.

So now the PR description links to that, saying so.

And if subcommand longs means ./exe sub, then this PR adds a test for alias conflicts.

clap supports subcommands to be flags, so you can use either foo or --foo. Named and long flag subcommands go through different code paths. It looks like long flag subcommands should work but we should make sure we have tests for each of these cases.

@SUPERCILEX
Copy link
Contributor Author

clap supports subcommands to be flags, so you can use either foo or --foo

I had no idea that was a thing, thanks. Where is it documented?

@epage
Copy link
Member

epage commented Jul 21, 2023

@SUPERCILEX
Copy link
Contributor Author

Thanks, seems to all work!

@SUPERCILEX
Copy link
Contributor Author

Sorry for completely dropping the ball on this, had to prioritize some other stuff. My nonsense tests should be fixed and long_flag inference actually works now.

@epage
Copy link
Member

epage commented Sep 25, 2023

Thanks for the follow up!

As a nit, would you be ok moving all tests to a single commit at the start of the PR, making them pass on their own, and then have the 2 fix commits on top that update the test to pass again with their change? This will help show that (1) the test is testing the right thing and (2) this does fix the issue.

@SUPERCILEX
Copy link
Contributor Author

As a nit, would you be ok moving all tests to a single commit at the start of the PR

Lol yeah I've even written about this before, done.

@epage
Copy link
Member

epage commented Sep 25, 2023

Thanks for your patience through this whole series and for getting this implemented!

@epage epage merged commit 3ac4404 into clap-rs:master Sep 25, 2023
22 checks passed
@SUPERCILEX
Copy link
Contributor Author

Awesome, thanks!

@epage
Copy link
Member

epage commented Sep 25, 2023

4.4.5 is now out

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.

infer_subcommands should not fail if the ambiguity is over aliases
2 participants