Prevent panic on flagSet access from custom BashComplete #946

unRob · 2019-11-25T19:25:41Z

Fixes #944

codecov · 2019-11-25T19:37:18Z

Codecov Report

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

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

unRob · 2019-11-25T19:49:59Z

command.go

@@ -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) {


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!

unRob · 2019-11-25T19:53:05Z

command.go

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


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.

⭐⭐⭐ (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

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?

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.

Depends on urfave/cli#946

coilysiren · 2019-11-27T03:25:39Z

command.go

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


⭐⭐⭐ (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

Add description to that function's docstring, and delete extraneous space

parse.go

coilysiren

love it! thanks so much!

saschagrunert

LGTM, thanks 🙏

Depends on urfave/cli#946

Prevent panic on flagSet access from custom BashComplete

90a62d7

Fixes urfave#944

unRob commented Nov 25, 2019

View reviewed changes

unRob marked this pull request as ready for review November 25, 2019 19:53

unRob requested a review from a team as a code owner November 25, 2019 19:53

unRob mentioned this pull request Nov 25, 2019

v2 bug: ctx.NArg() panics on Command.BashComplete #944

Closed

3 tasks

unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Nov 25, 2019

fix shell completion after update to urfave/cli@2.0.0

933c2ed

Depends on urfave/cli#946

coilysiren requested changes Nov 27, 2019

View reviewed changes

coilysiren assigned unRob Nov 27, 2019

Roberto Hidalgo added 2 commits November 27, 2019 11:42

Check for parsing errors within parse.go:parseIter

f3295e3

Add description to that function's docstring, and delete extraneous space

use shellComplete instead of ignoreErrors`, clean up

9e7226e

coilysiren reviewed Nov 28, 2019

View reviewed changes

parse.go Show resolved Hide resolved

coilysiren added 2 commits November 27, 2019 20:36

testing the tests

d733c54

revert testing change

233958c

coilysiren approved these changes Nov 28, 2019

View reviewed changes

coilysiren requested a review from saschagrunert November 28, 2019 05:23

saschagrunert approved these changes Nov 28, 2019

View reviewed changes

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

use urfave post urfave/cli#946

d5b1117

unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Dec 2, 2019

fix shell completion after update to urfave/cli@2.0.0

92b70c7

Depends on urfave/cli#946

unRob pushed a commit to blinkhealth/go-config-yourself that referenced this pull request Dec 2, 2019

use urfave post urfave/cli#946

3c26c55

coilysiren mentioned this pull request Dec 24, 2019

Release 2.1.0 #1013

Merged

unRob deleted the fix/issue-944/ctx-narg-panics-during-custom-bashcomplete branch March 18, 2020 05:41

VirrageS mentioned this pull request Mar 31, 2020

[v1] (backport) Prevent panic on flagSet access from custom BashComplete #1101

Merged

4 tasks

dependabot bot mentioned this pull request Mar 16, 2021

go-tool: bump github.com/urfave/cli from 1.19.1 to 1.22.5 Workiva/frugal#1508

Merged

This was referenced May 21, 2021

fix(deps): update module github.com/urfave/cli to v2 - autoclosed karlderkaefer/go-template-project#17

Closed

Update module github.com/urfave/cli to v2 - autoclosed drone-plugins/drone-s3#74

Closed

renovate bot mentioned this pull request Jun 11, 2021

chore(deps): update all (major) moul/assh#430

Open

1 task

renovate bot mentioned this pull request Jul 19, 2021

chore(deps): update all (major) moul/converter#60

Open

1 task

renovate bot mentioned this pull request Dec 20, 2022

fix(deps): update module github.com/urfave/cli to v2 chrisns/kubectl-passman#123

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent panic on flagSet access from custom BashComplete #946

Prevent panic on flagSet access from custom BashComplete #946

unRob commented Nov 25, 2019

codecov bot commented Nov 25, 2019 •

edited

unRob Nov 25, 2019

unRob Nov 25, 2019

coilysiren Nov 27, 2019 •

edited

coilysiren Nov 27, 2019

unRob Nov 27, 2019

coilysiren Nov 27, 2019 •

edited

coilysiren left a comment

saschagrunert left a comment

Prevent panic on flagSet access from custom BashComplete #946

Prevent panic on flagSet access from custom BashComplete #946

Conversation

unRob commented Nov 25, 2019

codecov bot commented Nov 25, 2019 • edited

Codecov Report

unRob Nov 25, 2019

Choose a reason for hiding this comment

unRob Nov 25, 2019

Choose a reason for hiding this comment

coilysiren Nov 27, 2019 • edited

Choose a reason for hiding this comment

coilysiren Nov 27, 2019

Choose a reason for hiding this comment

unRob Nov 27, 2019

Choose a reason for hiding this comment

coilysiren Nov 27, 2019 • edited

Choose a reason for hiding this comment

coilysiren left a comment

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 25, 2019 •

edited

coilysiren Nov 27, 2019 •

edited

coilysiren Nov 27, 2019 •

edited