Add a method to expose unknown flags back to the user #199
Conversation
Currently it is possible to ignore unknown flags (with
`FlagSet.ParseErrorsWhitelist.UnknownFlags`) but there is no way to find out
what they were after the fact.
Add a method which registers a slice into which all unknown arguments will be
accumulated.
Note that unknown short arguments which appear in a group (e.g. `-uuu`) will be
exploded (e.g. into `-u -u -u`) in this slice. Also any following value is
associated with the final option, e.g. `["-uuu", "foo"]` is interpreted as
`["-u", "-u", "-u", "foo"]`. This is consistent with the statement:
Single dashes signify a series of shorthand letters for flags. All but the
last shorthand letter must be boolean flags.
This is the reason why the call to `stripUnknownFlagValue` in
`parseSingleShortArg` becomes conditional on this being the last short arg in
the batch.
Add code to the existing `TestIgnoreUnknownFlags` (which already has
comprehensive set of the varities of possible unknown flags) to check this.
Also fixed a small cut-and-paste-o in the failure message of the test (use of
`f.ParseAll`).
Signed-off-by: Ian Campbell <[email protected]>
|
My immediate need for this became obsolete with docker/cli#1722 so feel free to close if there is no interest in this. |
|
@ijc I have some interest in this still, since the request was approved by @seemethere already can we merge this? |
|
I've moved onto other employment but I have no objections. I don't have merge privileges myself. |
|
@spf13 bump, can we get this merged or some review on what changes are needed? |
|
@spf13 Bump, it seems like this is ready to merge |
|
This is a really simple, and to my understanding, a missing piece to the unknown flags mechanism. This would easily fix spf13/cobra#739. |
|
+1 - I think this would help the cobra community alot! |
| // 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This response sums up my experience trying to contribute to this repo. I've waited two years for review. That's insane.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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 :)
|
Any idea when this could be merged? @eparis @therealmitchconnors @bep |
so a justification for not always storing them is that current behavior is to fail fast on the first unknown flag. it's true doing this "the other way" would still allow failing if anything wasn't parsed but it would fail after parsing the unknown flags - which is a behavior change that may have unexpected impact. other arguments for merging this PR are that at 51/49 preference isn't a large preference, this code is here and ready to go, and cobra is waiting on it :-) any thoughts on if we can get this merged in soon? |
| "--unknown7=unknown7value", | ||
| "--unknown8=unknown8value", | ||
| "--unknown6", "", | ||
| "-u", "-u", "-u", "-u", "-u", "", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
@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.
|
Would still be great for Cobra |
|
@underrun Your concern is covered by the whitelisting option @ijc As pointed out in my review, I think this PR breaks expected behavior cc; @marckhouzam @jpmcb |
@cornfeedhobo Thanks but please see #199 (comment): I'm not planning to do anything further with this PR at this point (maybe I should just close it?). If it isn't going to be merged as is then someone else will have to take it over and address the review. |
|
@ijc My apologies, I totally forgot about that comment. Thanks for the prompt reply. Cheers! |
|
@cornfeedhobo my full concern as laid out in the related cobra issue combined with my commend a month ago is not covered by whitelisting - or I don't think it is. Maybe you could explain how? @ijc - i'm willing to take this over if there are changes that could get it merged |
|
@underrun I modified this PR a little before merging it into my fork, so maybe you're right, but I'm pretty sure this is covered in this PR. Either way, you can look at my commit history and pull out the relevant changes if you are holding out hope that this will be reviewed by the repo maintainers. It specifically checks for the whitelist condition. |
|
The description says that -uuu foo is somehow treated specially. I'd have to look at things more closely. But if the intention of the user was the flag -u and the value uu, with a separate unrelated argument foo, all of that context would be lost by anything trying to look at unparsed flags right. So it would be impossible for the code handling these unknown flags to correctly handle things. Instead of using the arbitrary example above let's assume what the user typed was |
personally i think it's reasonable that if we are combing unknown argument parsing with allowance for concatenated short flags to disallow concatenating the argument of the last short flag with the list of short flags. but i think it'd also be possible to stop treating data as unknown short flags after hitting a short flag that is non-boolean? |
|
This just got a little more interesting. I decided to write out some test cases, and the first one I wrote seems to uncover a bug, if someone would like to verify: testCases := []struct {
args []string
wantFlags []string
wantArgs []string
wantUnknowns []string
}{
// Known flags
// StringP("name", "n", ...)
// BoolP("enabled", "E", ...)
// Unknown flags
// StringP("group", "g", ...)
// BoolP("hidden", "H", ...)
{
args: []string{"--name", "foo", "arg1", "--group", "bar", "arg2", "--enabled", "arg3", "--hidden", "arg4"},
wantFlags: []string{"--name", "foo", "--enabled", "true"},
wantUnknowns: []string{"--group", "bar", "--hidden"},
wantArgs: []string{"arg1", "arg2", "arg3", "arg4"},
},
} |
|
is that a bug or is that like ... impossible to differentiate with unknown flags where unknown flags can be either boolean or non-boolean? would this be fixed by disallowing both boolean and non-boolean unknown flags to be handled at the same time and giving users the choice to parse boolean unknown flags or non-boolean unknown flags? I don't like this option but maybe? another option would be to disallow boolean unknown flags except for the final flag (which mirrors short flag handling). |
@underrun Exactly. That is where this breaks down - do we assume they are values when no |
Currently it is possible to ignore unknown flags (with
FlagSet.ParseErrorsWhitelist.UnknownFlags) but there is no way to find outwhat they were after the fact.
Add a method which registers a slice into which all unknown arguments will be
accumulated.
Note that unknown short arguments which appear in a group (e.g.
-uuu) will beexploded (e.g. into
-u -u -u) in this slice. Also any following value isassociated with the final option, e.g.
["-uuu", "foo"]is interpreted as["-u", "-u", "-u", "foo"]. This is consistent with the statement:This is the reason why the call to
stripUnknownFlagValueinparseSingleShortArgbecomes conditional on this being the last short arg inthe batch.
Add code to the existing
TestIgnoreUnknownFlags(which already hascomprehensive set of the varities of possible unknown flags) to check this.
Also fixed a small cut-and-paste-o in the failure message of the test (use of
f.ParseAll).Signed-off-by: Ian Campbell [email protected]
For my use I'm actually only interested in the first unknown argument but this seemed like a more flexible approach which might be more widely usable. (I could probably get away with a flag "there were unknown args", although that wouldn't be so nice).