Prevent panic on flagSet access from custom BashComplete #946
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| @@ -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) { | |||
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Great idea!
- Changed the signature of
parseIterto accept a fourth parametershellComplete boolto deal with nil errors within. - Decided to keep the
shellCompletename, played withignoreErrorsinstead, but it felt like a misnomer sinceset.Parsewon't actually error on it. - Modified a couple of consumers to pass
shellCompletefrom their own context as well. - Added a couple of lines to the docstring, hope my wording conveys the use for passing that argument.
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 { | |||
Add description to that function's docstring, and delete extraneous space
unRob
deleted the
fix/issue-944/ctx-narg-panics-during-custom-bashcomplete
branch
Fixes #944