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: don't remove flag value that matches subcommand name #1781

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

brianpursley
Copy link
Contributor

When the command searches args to find the arg matching a particular subcommand name, it needs to ignore flag values, as it is possible that the value for a flag might match the name of the sub command.

This change improves argsMinusFirstX() to ignore flag values when it searches for the X to exclude from the result.

Fixes #1777

@github-actions github-actions bot added the size/L Denotes a PR that chanes 100-199 lines label Aug 19, 2022
command.go Show resolved Hide resolved
Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

This overall makes sense but since we'd be changing the API from
...
we'd have to wait for Cobra 2.0.0.

Disregard! I've been doing rust too long - these aren't exported

@yxxhero
Copy link

yxxhero commented Oct 24, 2022

any update?

@brianpursley
Copy link
Contributor Author

any update?

I don't think there are any outstanding questions or changes requested, so it waiting on further review or approval.

@yxxhero
Copy link

yxxhero commented Oct 24, 2022

@brianpursley Thanks for your reply. I'm waiting for this PR.

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

This looks correct to me and I think it is safe with respect to backwards-compatibility.
It will fix the broken case, which I don't see anyone expecting to work as it does now.

Since this PR requires a small rebase, I allowed myself to add two nits that hopefully @brianpursley won't mind addressing at the same time.

Thanks for your patience.

command.go Outdated Show resolved Hide resolved
command_test.go Outdated Show resolved Hide resolved
@marckhouzam marckhouzam added kind/bug A bug in cobra; unintended behavior area/flags-args Changes to functionality around command line flags and args labels Nov 8, 2022
When the command searches args to find the arg matching a
particular subcommand name, it needs to ignore flag values,
as it is possible that the value for a flag might match
the name of the sub command.

This change improves argsMinusFirstX() to ignore flag values
when it searches for the X to exclude from the result.
Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!
LGTM

@marckhouzam marckhouzam added the lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready label Nov 8, 2022
@marckhouzam marckhouzam merged commit 6b0bd30 into spf13:main Nov 8, 2022
@umarcor
Copy link
Contributor

umarcor commented Nov 12, 2022

@marckhouzam please milestone this!

@marckhouzam
Copy link
Collaborator

@marckhouzam please milestone this!

This was already milestoned 🙂

hoshsadiq pushed a commit to zulucmd/zulu that referenced this pull request May 16, 2023
When the command searches args to find the arg matching a
particular subcommand name, it needs to ignore flag values,
as it is possible that the value for a flag might match
the name of the sub command.

This change improves argsMinusFirstX() to ignore flag values
when it searches for the X to exclude from the result.

Fixes spf13/cobra#1777

Merge spf13/cobra#1781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/flags-args Changes to functionality around command line flags and args kind/bug A bug in cobra; unintended behavior lgtm Denotes "looks good to me" from maintainers and signals other collaboratores that a PR is ready size/L Denotes a PR that chanes 100-199 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flag value misparsed as subcommand
5 participants