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

Prevent panic on flagSet access from custom BashComplete #946

Merged
merged 5 commits into from
Nov 28, 2019
Merged

Prevent panic on flagSet access from custom BashComplete #946

merged 5 commits into from
Nov 28, 2019

Conversation

unRob
Copy link

@unRob unRob commented Nov 25, 2019

Fixes #944

@codecov
Copy link

codecov bot commented Nov 25, 2019

Codecov Report

Merging #946 into master will increase coverage by 0.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #946      +/-   ##
==========================================
+ Coverage   72.89%   73.37%   +0.47%     
==========================================
  Files          32       32              
  Lines        2439     2441       +2     
==========================================
+ Hits         1778     1791      +13     
+ Misses        550      540      -10     
+ Partials      111      110       -1
Impacted Files Coverage Δ
parse.go 90.24% <100%> (+0.5%) ⬆️
app.go 77.07% <100%> (ø) ⬆️
command.go 82.7% <100%> (+1.5%) ⬆️
help.go 85.22% <0%> (+5.11%) ⬆️

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 4805bd1...233958c. Read the comment docs.

@@ -174,7 +174,7 @@ func (c *Command) useShortOptionHandling() bool {
return c.UseShortOptionHandling
}

func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
func (c *Command) parseFlags(args Args, shellComplete bool) (*flag.FlagSet, error) {
Copy link
Author

Choose a reason for hiding this comment

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

Not quite sure if this name/concept is right as an argument, kept it for the sake of consistency but happy to adjust if someone has a better idea!

command.go Outdated
@@ -185,7 +185,8 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
}

err = parseIter(set, c, args.Tail())
if err != nil {
// Continue parsing flags on failure during shell completion
if err != nil && !shellComplete {
Copy link
Author

Choose a reason for hiding this comment

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

This basically allows for the context to continue to be parsed. Malformed flags will still be ignored and not part of the set, which is what v2 pre-release builds did.

Copy link
Member

@coilysiren coilysiren Nov 27, 2019

Choose a reason for hiding this comment

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

⭐⭐⭐ (must change) @unRob can you add in here a short description of what parseIter does, and a detailed description of why it's ok to skip errors from it when doing shell completion?

FYI cc @rliebz

Copy link
Member

Choose a reason for hiding this comment

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

OH! Actually! I have a better idea: can you change this code to pass shellComplete into the parseIter function, ignore the errors inside the function itself, and add the descriptions I just requested inside the function's docstring?

Copy link
Author

Choose a reason for hiding this comment

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

Great idea!

  • Changed the signature of parseIter to accept a fourth parameter shellComplete bool to deal with nil errors within.
  • Decided to keep the shellComplete name, played with ignoreErrors instead, but it felt like a misnomer since set.Parse won't actually error on it.
  • Modified a couple of consumers to pass shellComplete from their own context as well.
  • Added a couple of lines to the docstring, hope my wording conveys the use for passing that argument.

@unRob unRob marked this pull request as ready for review November 25, 2019 19:53
@unRob unRob requested a review from a team as a code owner November 25, 2019 19:53
unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Nov 25, 2019
command.go Outdated
@@ -185,7 +185,8 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) {
}

err = parseIter(set, c, args.Tail())
if err != nil {
// Continue parsing flags on failure during shell completion
if err != nil && !shellComplete {
Copy link
Member

@coilysiren coilysiren Nov 27, 2019

Choose a reason for hiding this comment

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

⭐⭐⭐ (must change) @unRob can you add in here a short description of what parseIter does, and a detailed description of why it's ok to skip errors from it when doing shell completion?

FYI cc @rliebz

Roberto Hidalgo added 2 commits November 27, 2019 11:42
Add description to that function's docstring, and delete extraneous space
parse.go Show resolved Hide resolved
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.

love it! thanks so much!

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, thanks 🙏

@coilysiren coilysiren merged commit 796ffdc into urfave:master Nov 28, 2019
unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Dec 2, 2019
unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Dec 2, 2019
unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Dec 2, 2019
@coilysiren coilysiren mentioned this pull request Dec 24, 2019
@unRob unRob deleted the fix/issue-944/ctx-narg-panics-during-custom-bashcomplete branch March 18, 2020 05:41
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.

v2 bug: ctx.NArg() panics on Command.BashComplete
3 participants