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

Bugfix: Remove version flag from commands with subcommands #1153

Merged
merged 4 commits into from Jun 23, 2020

Conversation

Irioth
Copy link
Contributor

@Irioth Irioth commented Jun 17, 2020

What type of PR is this?

  • bug
  • cleanup
  • documentation
  • feature

What this PR does / why we need it:

This PR removes unexpected version flag on commands with subcommands.

Which issue(s) this PR fixes:

Fixes #1073

Testing

test added

Release Notes

fix: remove unexpected version flag on commands with subcommands

@Irioth Irioth requested a review from a team as a code owner June 17, 2020 22:04
@Irioth Irioth requested review from rliebz and asahasrabuddhe and removed request for a team June 17, 2020 22:04
Copy link
Member

@rliebz rliebz left a comment

Choose a reason for hiding this comment

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

Looks good, this change makes sense to me.

Since this is targeting #1073, would you mind adding a test case demonstrating that it is now possible to add a -v flag to sub-commands?

command_test.go Outdated
HideHelp: true,
Action: func(c *Context) error {
if len(c.App.VisibleFlags()) != 0 {
t.Fatalf("unexpected flag on command")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Since there's no formatting here, t.Fatal is more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@Irioth
Copy link
Contributor Author

Irioth commented Jun 18, 2020

Looks good, this change makes sense to me.

Since this is targeting #1073, would you mind adding a test case demonstrating that it is now possible to add a -v flag to sub-commands?

Sure, added such one.

Copy link
Member

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

I just tested it, and apparently the --version flag does nothing on commands and subcommands? So this PR LGTM.

@coilysiren coilysiren requested a review from rliebz June 23, 2020 15:48
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@coilysiren coilysiren merged commit 9047f3b into urfave:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants