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 urfave/cli as a code owner Jun 17, 2020
@Irioth Irioth requested review from rliebz and asahasrabuddhe and removed request for urfave/cli Jun 17, 2020
Copy link
Member

@rliebz rliebz left a comment

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?

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

This comment has been minimized.

@rliebz

rliebz Jun 18, 2020
Member

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

This comment has been minimized.

@Irioth

Irioth Jun 18, 2020
Author Contributor

fixed

@Irioth
Copy link
Contributor Author

@Irioth 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

@lynncyrin lynncyrin left a comment

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

@lynncyrin lynncyrin requested a review from rliebz Jun 23, 2020
Copy link
Member

@saschagrunert saschagrunert left a comment

LGTM

@lynncyrin lynncyrin merged commit 9047f3b into urfave:master Jun 23, 2020
12 checks passed
12 checks passed
ubuntu-latest @ Go 1.12
Details
ubuntu-latest @ Go 1.13
Details
ubuntu-latest @ Go 1.14
Details
macos-latest @ Go 1.12
Details
macos-latest @ Go 1.13
Details
macos-latest @ Go 1.14
Details
windows-latest @ Go 1.12
Details
windows-latest @ Go 1.13
Details
windows-latest @ Go 1.14
Details
test-docs
Details
codecov/patch 100.00% of diff hit (target 73.27%)
Details
codecov/project 73.27% (+0.00%) compared to 4f74020
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants