Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 60 additions & 2 deletions flag.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ const (
type ParseErrorsAllowlist struct {
// UnknownFlags will ignore unknown flags errors and continue parsing rest of the flags
UnknownFlags bool
// PassUnknownFlagsToArgs will treat unknown flags as non-flag arguments.
// Combined shorthand flags mixed with known ones and unknown ones results
// combined flags only with unknown ones.
// E.g. -fghi results -gh if only `f` and `i` are known.
PassUnknownFlagsToArgs bool
}

// NormalizedName is a flag name that has been normalized according to rules
Expand Down Expand Up @@ -967,6 +972,17 @@ func stripUnknownFlagValue(args []string) []string {
return nil
}

// errUnknownFlag is used for internal unknown flag handling.
type unknownFlagError struct {
// UnknownFlags is flags that are unknown and unprocessed.
// It depends on the context whether this has a prefix like '-' or '--'.
UnknownFlags string
}

func (e *unknownFlagError) Error() string {
return fmt.Sprintf("unknown flag: %v", e.UnknownFlags)
}

func (f *FlagSet) parseLongArg(s string, args []string, fn parseFunc) (a []string, err error) {
a = args
name := s[2:]
Expand All @@ -985,6 +1001,11 @@ func (f *FlagSet) parseLongArg(s string, args []string, fn parseFunc) (a []strin
f.usage()
return a, ErrHelp
case f.ParseErrorsAllowlist.UnknownFlags:
if f.ParseErrorsAllowlist.PassUnknownFlagsToArgs {
return a, &unknownFlagError{
UnknownFlags: s,
}
}
Comment on lines 1003 to +1008
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't spot this before, but putting this branch here instead of in its own case means users will have to specify both this option and UnknownFlags: true for this to have any effect. Is that intentional?

Either way, it seems to me like this PR introduces a new mode to handle unknown flags where there are already two. Before this change, depending on the value of UnknownFlags an unknown flag would either cause an error or be ignored (but consumed). Now, it can also be ignored but not consumed (and instead passed to Args), but controlling this with two boolean feature flags results in an API where there's a combination of values that doesn't make sense.

Maybe it would be better to change UnknownFlags to an enum with constants for ErrorOnUnknown, IgnoreUnknown and PassUnknownToArgs instead? I realize this means the change is breaking, but we have a bunch of ideas lined up that would require breaking changes, so I think it's time to start thinking about what the path to 2.0 looks like anyway. Would you be open to this idea?

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense.

I believe switching to enum can be achieved preserving backward compatibility. Created #440 . But that requires complicated codes for backward compatibility, and it's OK to wait for 2.0.

// --unknown=unknownval arg ...
// we do not want to lose arg in this case
if len(split) >= 2 {
Expand Down Expand Up @@ -1047,6 +1068,18 @@ func (f *FlagSet) parseSingleShortArg(shorthands string, args []string, fn parse
// we do not want to lose arg in this case
if len(shorthands) > 2 && shorthands[1] == '=' {
outShorts = ""
if f.ParseErrorsAllowlist.PassUnknownFlagsToArgs {
err = &unknownFlagError{
UnknownFlags: shorthands,
}
}
return
}

if f.ParseErrorsAllowlist.PassUnknownFlagsToArgs {
err = &unknownFlagError{
UnknownFlags: shorthands[0:1],
}
return
}

Expand Down Expand Up @@ -1102,14 +1135,32 @@ func (f *FlagSet) parseSingleShortArg(shorthands string, args []string, fn parse
func (f *FlagSet) parseShortArg(s string, args []string, fn parseFunc) (a []string, err error) {
a = args
shorthands := s[1:]
var errUnknownFlagAll *unknownFlagError

// "shorthands" can be a series of shorthand letters of flags (e.g. "-vvv").
for len(shorthands) > 0 {
shorthands, a, err = f.parseSingleShortArg(shorthands, args, fn)
if err != nil {
return
if errUnknownFlag, ok := err.(*unknownFlagError); ok {
// this means f.ParseErrorsAllowlist.UnknownFlags
// and f.ParseErrorsAllowlist.PassUnknownFlagsToArgs are set.
if errUnknownFlagAll == nil {
errUnknownFlagAll = &unknownFlagError{
UnknownFlags: "-",
}
}

errUnknownFlagAll.UnknownFlags = errUnknownFlagAll.UnknownFlags +
errUnknownFlag.UnknownFlags
err = nil
} else {
return
}
}
}
if errUnknownFlagAll != nil {
err = errUnknownFlagAll
}

return
}
Expand Down Expand Up @@ -1139,7 +1190,14 @@ func (f *FlagSet) parseArgs(args []string, fn parseFunc) (err error) {
args, err = f.parseShortArg(s, args, fn)
}
if err != nil {
return
if errUnknownFlag, ok := err.(*unknownFlagError); ok {
// this means f.ParseErrorsAllowlist.UnknownFlags
// and f.ParseErrorsAllowlist.PassUnknownFlagsToArgs are set.
f.args = append(f.args, errUnknownFlag.UnknownFlags)
err = nil
} else {
return
}
}
}
return
Expand Down
110 changes: 110 additions & 0 deletions flag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,112 @@ func testParseWithUnknownFlags(f *FlagSet, t *testing.T) {
}
}

func testParseWithUnknownFlagsAndPassToArgs(f *FlagSet, t *testing.T) {
if f.Parsed() {
t.Fatal("f.Parse() = true before Parse")
}
f.ParseErrorsAllowlist.UnknownFlags = true
f.ParseErrorsAllowlist.PassUnknownFlagsToArgs = true
f.SetInterspersed(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",
// -f and -g is unknown
"-fcgs=xx",
"--stringz=something",
"--unknown1",
"unknown1Value",
"-d=true",
"-x",
"--unknown2=unknown2Value",
"-u=unknown3Value",
"-p",
"unknown4Value",
"-q", //another unknown with bool value
"-y",
"ee",
"--unknown7=unknown7value",
"--stringo=ovalue",
"--unknown8=unknown8value",
"--boole",
"--unknown6",
"",
"-uuuuu",
"",
"--unknown10",
"--unknown11",
"arg0",
"arg1",
}
want := []string{
"boola", "true",
"boolb", "true",
"boolc", "true",
"stringa", "xx",
"stringz", "something",
"boold", "true",
"stringx", "1",
"stringy", "ee",
"stringo", "ovalue",
"boole", "true",
}
wantArgs := []string{
"-fg",
"--unknown1",
"unknown1Value",
"--unknown2=unknown2Value",
"-u=unknown3Value",
"-p",
"unknown4Value",
"-q", //another unknown with bool value
"--unknown7=unknown7value",
"--unknown8=unknown8value",
"--unknown6",
"",
"-uuuuu",
"",
"--unknown10",
"--unknown11",
"arg0",
"arg1",
}
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)
}
if !reflect.DeepEqual(f.Args(), wantArgs) {
t.Errorf("f.ParseAll() fail to restore the args")
t.Errorf("Got: %v", f.Args())
t.Errorf("Want: %v", wantArgs)
}
}

func TestShorthand(t *testing.T) {
f := NewFlagSet("shorthand", ContinueOnError)
if f.Parsed() {
Expand Down Expand Up @@ -652,6 +758,10 @@ func TestIgnoreUnknownFlags(t *testing.T) {
testParseWithUnknownFlags(GetCommandLine(), t)
}

func TestIgnoreUnknownFlagsAndPassToArgs(t *testing.T) {
ResetForTesting(func() { t.Error("bad parse") })
testParseWithUnknownFlagsAndPassToArgs(GetCommandLine(), t)
}
func TestFlagSetParse(t *testing.T) {
testParse(NewFlagSet("test", ContinueOnError), t)
}
Expand Down
Loading