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

Add HideHelpCommand #1083

Merged
merged 1 commit into from
Mar 6, 2020
Merged

Add HideHelpCommand #1083

merged 1 commit into from
Mar 6, 2020

Conversation

AkihiroSuda
Copy link
Contributor

@AkihiroSuda AkihiroSuda commented Mar 5, 2020

What type of PR is this?

  • feature

What this PR does / why we need it:

While HideHelp hides both help command and --help flag, HideHelpCommand
only hides help command and leave --help flag as-is.

The behavior of HideHelp is untouched in this commit.

Which issue(s) this PR fixes:

Fixes #523
Replaces #636

Testing

go test -v -run TestHideHelpCommand

Release Notes

Added `HideHelpCommand`. While `HideHelp` hides both `help` command and `--help` flag, `HideHelpCommand` only hides `help` command and leave `--help` flag as-is.

@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

Merging #1083 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1083      +/-   ##
==========================================
+ Coverage   72.91%   73.18%   +0.27%     
==========================================
  Files          33       33              
  Lines        2481     2480       -1     
==========================================
+ Hits         1809     1815       +6     
+ Misses        565      559       -6     
+ Partials      107      106       -1
Impacted Files Coverage Δ
flag.go 86.39% <ø> (+0.16%) ⬆️
app.go 81.22% <100%> (+2.5%) ⬆️
command.go 82.83% <100%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b7e4e0...75e7c52. Read the comment docs.

@AkihiroSuda AkihiroSuda force-pushed the hide-help-command branch 2 times, most recently from 2426f97 to 3e5678c Compare March 5, 2020 07:36
@AkihiroSuda
Copy link
Contributor Author

Any chance to get v2.2.0 released soon with this?

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, thanks for pulling this together.

I think the only thing left is getting the coverage check passing.

app.go Outdated Show resolved Hide resolved
@AkihiroSuda AkihiroSuda force-pushed the hide-help-command branch 4 times, most recently from 4604362 to 2129edf Compare March 5, 2020 15:26
While `HideHelp` hides both `help` command and `--help` flag, `HideHelpCommand`
only hides `help` command and leave `--help` flag as-is.

The behavior of `HideHelp` is untouched in this commit.

Fix urfave#523
Replace urfave#636

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Comment on lines -333 to -342
// append help to commands
if len(a.Commands) > 0 {
if a.Command(helpCommand.Name) == nil && !a.HideHelp {
a.appendCommand(helpCommand)

if HelpFlag != nil {
a.appendFlag(HelpFlag)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

(blocking question) this code is being removed because it was duplicated in Setup above, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and because of the duplication, this code path was not executed and could not be covered with tests: #1083 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Nice find!

@coilysiren
Copy link
Member

Any chance to get v2.2.0 released soon with this?

I can write up the release notes this weekend probs

@coilysiren coilysiren requested a review from rliebz March 6, 2020 08:09
@rliebz rliebz merged commit d648edd into urfave:master Mar 6, 2020
@coilysiren coilysiren mentioned this pull request Mar 9, 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
Development

Successfully merging this pull request may close these issues.

Don't want any commands in the CLI interface (including help)
3 participants