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 ability to ignore unknown flags #160

Merged
merged 5 commits into from Mar 30, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 57 additions & 6 deletions flag.go
Expand Up @@ -123,6 +123,12 @@ const (
PanicOnError
)

// ParseErrorsWhitelist defines the parsing errors that can be ignored
type ParseErrorsWhitelist struct {
// UnknownFlags will ignore unknown flags errors and continue parsing rest of the flags
UnknownFlags bool
}

// NormalizedName is a flag name that has been normalized according to rules
// for the FlagSet (e.g. making '-' and '_' equivalent).
type NormalizedName string
Expand All @@ -138,6 +144,9 @@ type FlagSet struct {
// help/usage messages.
SortFlags bool

// ParseErrorsWhitelist is used to configure a whitelist of errors
ParseErrorsWhitelist ParseErrorsWhitelist

name string
parsed bool
actual map[NormalizedName]*Flag
Expand Down Expand Up @@ -896,6 +905,25 @@ func (f *FlagSet) usage() {
}
}

//--unknown (args will be empty)
//--unknown --next-flag ... (args will be --next-flag ...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if I passed --unknown=bob --next-flag. I think this will strip --next-flag, but I don't think it should.

Copy link
Author

Choose a reason for hiding this comment

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

this function just strips value for unknown flag, the flag itself is not included in args.

for --unknown=value --next-flag ... args will be --next-flag ...

added the testcase to test this explicitly

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really think you need to know len(split) (from parseLongArg) and to use that. What if it was --unknown=value subcmd. You would strip subcmd which is really not what the user wants.

Clearly it is a heuristic to know what to strip and what not to strip for an 'unknown' but I think we want to be as careful as we can...

Copy link
Author

Choose a reason for hiding this comment

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

oh yes, thats right. i would consider this a bug :)

fixed. thank you for pointing out.

//--unknown arg ... (args will be arg ...)
func stripUnknownFlagValue(args []string) []string {
if len(args) == 0 {
//--unknown
return args
}

first := args[0]
if first[0] == '-' {
//--unknown --next-flag ...
return args
}

//--unknown arg ... (args will be arg ...)
return args[1:]
}

func (f *FlagSet) parseLongArg(s string, args []string, fn parseFunc) (a []string, err error) {
a = args
name := s[2:]
Expand All @@ -907,13 +935,24 @@ func (f *FlagSet) parseLongArg(s string, args []string, fn parseFunc) (a []strin
split := strings.SplitN(name, "=", 2)
name = split[0]
flag, exists := f.formal[f.normalizeFlagName(name)]

if !exists {
if name == "help" { // special case for nice help message.
switch {
case name == "help":
f.usage()
return a, ErrHelp
case f.ParseErrorsWhitelist.UnknownFlags:
// --unknown=unknownval arg ...
// we do not want to lose arg in this case
if len(split) >= 2 {
return a, nil
}

return stripUnknownFlagValue(a), nil
default:
err = f.failf("unknown flag: --%s", name)
return
}
err = f.failf("unknown flag: --%s", name)
return
}

var value string
Expand Down Expand Up @@ -951,13 +990,25 @@ func (f *FlagSet) parseSingleShortArg(shorthands string, args []string, fn parse

flag, exists := f.shorthands[c]
if !exists {
if c == 'h' { // special case for nice help message.
switch {
case c == 'h':
f.usage()
err = ErrHelp
return
case f.ParseErrorsWhitelist.UnknownFlags:
// '-f=arg arg ...'
// we do not want to lose arg in this case
if len(shorthands) > 2 && shorthands[1] == '=' {
outShorts = ""
return
}

outArgs = stripUnknownFlagValue(outArgs)
return
default:
err = f.failf("unknown shorthand flag: %q in -%s", c, shorthands)
return
}
err = f.failf("unknown shorthand flag: %q in -%s", c, shorthands)
return
}

var value string
Expand Down
78 changes: 77 additions & 1 deletion flag_test.go
Expand Up @@ -389,7 +389,78 @@ func testParseAll(f *FlagSet, t *testing.T) {
}
if !reflect.DeepEqual(got, want) {
t.Errorf("f.ParseAll() fail to restore the args")
t.Errorf("Got: %v", got)
t.Errorf("Got: %v", got)
t.Errorf("Want: %v", want)
}
}

func testParseWithUnknownFlags(f *FlagSet, t *testing.T) {
if f.Parsed() {
t.Error("f.Parse() = true before Parse")
}
f.ParseErrorsWhitelist.UnknownFlags = true

f.BoolP("boola", "a", false, "bool value")
f.BoolP("boolb", "b", false, "bool2 value")
f.BoolP("boolc", "c", false, "bool3 value")
f.BoolP("boold", "d", false, "bool4 value")
f.BoolP("boole", "e", false, "bool4 value")
f.StringP("stringa", "s", "0", "string value")
f.StringP("stringz", "z", "0", "string value")
f.StringP("stringx", "x", "0", "string value")
f.StringP("stringy", "y", "0", "string value")
f.StringP("stringo", "o", "0", "string value")
f.Lookup("stringx").NoOptDefVal = "1"
args := []string{
"-ab",
"-cs=xx",
"--stringz=something",
"--unknown1",
"unknown1Value",
"-d=true",
"-x",
"--unknown2=unknown2Value",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this test need to cover the case where --unknown=value is followed by --known

Copy link
Author

Choose a reason for hiding this comment

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

added the testcase, thanks

"-u=unknown3Value",
"-p",
"unknown4Value",
"-q", //another unknown with bool value
"-y",
"ee",
"--unknown7=unknown7value",
"--stringo=ovalue",
"--unknown8=unknown8value",
"--boole",
"--unknown6",
}
want := []string{
"boola", "true",
"boolb", "true",
"boolc", "true",
"stringa", "xx",
"stringz", "something",
"boold", "true",
"stringx", "1",
"stringy", "ee",
"stringo", "ovalue",
"boole", "true",
}
got := []string{}
store := func(flag *Flag, value string) error {
got = append(got, flag.Name)
if len(value) > 0 {
got = append(got, value)
}
return nil
}
if err := f.ParseAll(args, store); err != nil {
t.Errorf("expected no error, got %s", err)
}
if !f.Parsed() {
t.Errorf("f.Parse() = false after Parse")
}
if !reflect.DeepEqual(got, want) {
t.Errorf("f.ParseAll() fail to restore the args")
t.Errorf("Got: %v", got)
t.Errorf("Want: %v", want)
}
}
Expand Down Expand Up @@ -500,6 +571,11 @@ func TestParseAll(t *testing.T) {
testParseAll(GetCommandLine(), t)
}

func TestIgnoreUnknownFlags(t *testing.T) {
ResetForTesting(func() { t.Error("bad parse") })
testParseWithUnknownFlags(GetCommandLine(), t)
}

func TestFlagSetParse(t *testing.T) {
testParse(NewFlagSet("test", ContinueOnError), t)
}
Expand Down