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.

Sign up for GitHub

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 a method to expose unknown flags back to the user #199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add a method to expose unknown flags back to the user #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 35 additions & 4 deletions flag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ const (

// 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 will ignore unknown flags errors and continue
// parsing rest of the flags. Consider using
// SetUnknownFlagsSlice if you need to know which unknown
// flags occured
UnknownFlags bool
}

Expand Down Expand Up @@ -163,6 +166,7 @@ type FlagSet struct {
output io.Writer // nil means stderr; use out() accessor
interspersed bool // allow interspersed option/non-option args
normalizeNameFunc func(f *FlagSet, name string) NormalizedName
unknownflags *[]string

addedGoFlagSets []*goflag.FlagSet
}
Expand Down Expand Up @@ -915,10 +919,17 @@ func (f *FlagSet) usage() {
}
}

func (f *FlagSet) addUnknownFlag(s string) {
if f.unknownflags == nil {
return
}
*f.unknownflags = append(*f.unknownflags, s)
}

//--unknown (args will be empty)
//--unknown --next-flag ... (args will be --next-flag ...)
//--unknown arg ... (args will be arg ...)
func stripUnknownFlagValue(args []string) []string {
func (f *FlagSet) stripUnknownFlagValue(args []string) []string {
if len(args) == 0 {
//--unknown
return args
Expand All @@ -932,6 +943,7 @@ func stripUnknownFlagValue(args []string) []string {

//--unknown arg ... (args will be arg ...)
if len(args) > 1 {
f.addUnknownFlag(args[0])
return args[1:]
}
return nil
Expand All @@ -955,13 +967,14 @@ func (f *FlagSet) parseLongArg(s string, args []string, fn parseFunc) (a []strin
f.usage()
return a, ErrHelp
case f.ParseErrorsWhitelist.UnknownFlags:
f.addUnknownFlag(s)
// --unknown=unknownval arg ...
// we do not want to lose arg in this case
if len(split) >= 2 {
return a, nil
}

return stripUnknownFlagValue(a), nil
return f.stripUnknownFlagValue(a), nil
default:
err = f.failf("unknown flag: --%s", name)
return
Expand Down Expand Up @@ -1013,11 +1026,15 @@ func (f *FlagSet) parseSingleShortArg(shorthands string, args []string, fn parse
// '-f=arg arg ...'
// we do not want to lose arg in this case
if len(shorthands) > 2 && shorthands[1] == '=' {
f.addUnknownFlag("-" + shorthands)
outShorts = ""
return
}

outArgs = stripUnknownFlagValue(outArgs)
f.addUnknownFlag("-" + string(c))
if len(outShorts) == 0 {
outArgs = f.stripUnknownFlagValue(outArgs)
}
return
default:
err = f.failf("unknown shorthand flag: %q in -%s", c, shorthands)
Expand Down Expand Up @@ -1171,6 +1188,13 @@ func (f *FlagSet) Parsed() bool {
return f.parsed
}

// SetUnknownFlagsSlice arranges to append any unknown flags to the
// given slice. This requires ParseErrorsWhitelist.UnknownFlags to be
// set so that parsing does not abort on the first unknown flag.
func (f *FlagSet) SetUnknownFlagsSlice(s *[]string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm debating if I like this API or if we should just always store them in th flagset and the user call GetUnknownFlagSlice() to fetch from our internal storage. I feel ike 51/49 towards using GetUnknownFlagSlice(). But why did you go this way?

Copy link
Author

Choose a reason for hiding this comment

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

I'm afraid I don't recall what I was thinking last week, let alone 20 months ago ;-)

My motivation for needing this ended up being going away due to other changes needed in the consuming code base and I've also since change jobs. I'm happy if someone else wants to carry this change though.

Copy link

Choose a reason for hiding this comment

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

This response sums up my experience trying to contribute to this repo. I've waited two years for review. That's insane.

Copy link

Choose a reason for hiding this comment

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

The cobra community would like this (or something like this) so that we can have an api access to unknown flags. Ether way would be fine as long as we can get them!

Copy link

@cornfeedhobo cornfeedhobo Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm finally merging down a few of these PRs into my fork and I'll update the readme to explain how to override the dependency when using cobra.

Edit: I've published my fork and will be working backwards through old PRs in the coming days, then I'll get to this one. It'd be great to get this into cobra or finally get this repo some love.

Copy link

Choose a reason for hiding this comment

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

@jpmcb Okay. I've made some changes to the way this was implemented and merged it into my fork. I hope that suits your needs, but feel free to open any issues and I'll jump on them.

@ijc Thanks for putting this together and making it easy!

Copy link

Choose a reason for hiding this comment

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

Thanks for putting a fork together. However, I think it would be an anti-goal of the cobra project to fracture viper, pflags, and cobra by introducing a fork as a core dependency. Cobra is really just a wrapper for pflags and viper. So ideally, I'd like if this could get some love and be merged

Copy link

Choose a reason for hiding this comment

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

@jpmcb Yeah, I'd like that too, but I don't want to wait another 2 years for a response. No one has an obligation to use my fork, but I'm movin' on :)

f.unknownflags = s
}

// Parse parses the command-line flags from os.Args[1:]. Must be called
// after all flags are defined and before flags are accessed by the program.
func Parse() {
Expand All @@ -1196,6 +1220,13 @@ func Parsed() bool {
return CommandLine.Parsed()
}

// SetUnknownFlagsSlice arranges to append any unknown flags to the
// given slice. This requires ParseErrorsWhitelist.UnknownFlags to be
// set so that parsing does not abort on the first unknown flag.
func SetUnknownFlagsSlice(s *[]string) {
CommandLine.SetUnknownFlagsSlice(s)
}

// CommandLine is the default set of command-line flags, parsed from os.Args.
var CommandLine = NewFlagSet(os.Args[0], ExitOnError)

Expand Down
22 changes: 21 additions & 1 deletion flag_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,8 @@ func testParseWithUnknownFlags(f *FlagSet, t *testing.T) {
t.Error("f.Parse() = true before Parse")
}
f.ParseErrorsWhitelist.UnknownFlags = true
var unknownFlags []string
f.SetUnknownFlagsSlice(&unknownFlags)

f.BoolP("boola", "a", false, "bool value")
f.BoolP("boolb", "b", false, "bool2 value")
Expand Down Expand Up @@ -449,6 +451,19 @@ func testParseWithUnknownFlags(f *FlagSet, t *testing.T) {
"stringo", "ovalue",
"boole", "true",
}
wantUnknowns := []string{
"--unknown1", "unknown1Value",
"--unknown2=unknown2Value",
"-u=unknown3Value",
"-p", "unknown4Value",
"-q",
"--unknown7=unknown7value",
"--unknown8=unknown8value",
"--unknown6", "",
"-u", "-u", "-u", "-u", "-u", "",
Copy link

@cornfeedhobo cornfeedhobo Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I realize this is brought up in the PR description, but I keep thinking about this line. If u is an unknown shorthand, everything following it could be a value, which matches the support syntax in the readme

// mixed
-abcs "hello"
-absd="hello"
-abcs1234

It doesn't seem right to assume they are concatenated shorthands.

You can see a working gist here.

Copy link

Choose a reason for hiding this comment

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

I strongly disagree that concatenating short flags and values should be supported by anything. It is very unexpected to me that this should work where valid short flags are t and o and the expected value for o is foobar

$ go run main.go -tofoobar

This should fail to work both because it's super confusing and terrible design and because it adds a high level of complexity at the flag parsing layer to support both this feature and unknown short flags.

Copy link

Choose a reason for hiding this comment

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

@underrun well, it's a pretty old pattern, so I think you'll have to take that up with history. The fact remains that it's a syntax currently supported, and the preservation of unknown flags should be as naive as possible so that the resulting slice can be manually passed to Parse.

Copy link

Choose a reason for hiding this comment

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

i don't see how having unknown flags can work while allowing concatenated short flags and values though. it feels like it's reasonable to disallow this if unknown flags are requred. no?

Copy link

@cornfeedhobo cornfeedhobo Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@underrun I'm not sure I fully follow, but you did help me stumble on another bug with this implementation; if the unknown flag has a value, this current implementation will swallow the value and consider it an argument, despite us having a test for this 🤔 .

Works: -Dfoobar
Doesn't work: -D foobar, --stringVal foobar

Copy link

Choose a reason for hiding this comment

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

@underrun I think I better understand your comment now. Essentially this comes down to an inability to surface values that are separated by a space, because they are indistinguishable from arguments, and there is no way to determine if it's associated to an unknown flag type. A best effort is all that can be done, and that is achieved if the user includes = when assigning a value to an unknown.

"--unknown10",
"--unknown11",
}
got := []string{}
store := func(flag *Flag, value string) error {
got = append(got, flag.Name)
Expand All @@ -464,10 +479,15 @@ func testParseWithUnknownFlags(f *FlagSet, t *testing.T) {
t.Errorf("f.Parse() = false after Parse")
}
if !reflect.DeepEqual(got, want) {
t.Errorf("f.ParseAll() fail to restore the args")
t.Errorf("f.Parse() failed to parse with unknown args")
t.Errorf("Got: %v", got)
t.Errorf("Want: %v", want)
}
if !reflect.DeepEqual(unknownFlags, wantUnknowns) {
t.Errorf("f.Parse() failed to enumerate the unknown args args")
t.Errorf("Got: %v", unknownFlags)
t.Errorf("Want: %v", wantUnknowns)
}
}

func TestShorthand(t *testing.T) {
Expand Down