pflag
pflag copied to clipboard
spf13
Add a method to expose unknown flags back to the user
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]
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).
My immediate need for this became obsolete with https://github.com/docker/cli/pull/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?
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! @eparis @therealmitchconnors @bep any chance we could get this looked at? Thanks much!
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?
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?
@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
@ijc As pointed out in my review, I think this PR breaks expected behavior
@cornfeedhobo Thanks but please see https://github.com/spf13/pflag/pull/199#discussion_r475735582: 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 -cblue draw. And this was supposed to mean to something handling unknown flags that the color is supposed to be blue and we're supposed to draw that color. Breaking this into a slice with six different entries seems problematic. You have no idea if the user typed -c -b -l -u -e draw, or what was actually typed. Does this ambiguity not seem problematic?
Instead of using the arbitrary example above let's assume what the user typed was -cblue draw. And this was supposed to mean to something handling unknown flags that the color is supposed to be blue and we're supposed to draw that color. Breaking this into a slice with six different entries seems problematic. You have no idea if the user typed -c -b -l -u -e draw, or what was actually typed. Does this ambiguity not seem problematic?
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"},
},
}
flag_test.go:674: Got: [arg1 arg2 arg3]
flag_test.go:675: Want: [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).
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?
@underrun Exactly. That is where this breaks down - do we assume they are values when no = is used, or do we assume they are arguments, or do we leave that to the developer to toggle through a setting(s)?
I ran into this too, and found this issue. If anyone else runs into this, there is another approach of making the unknown flags known. This POC happened to work for me, maybe it will work for others.
package main
import (
"flag"
"fmt"
"os"
"strings"
"github.com/spf13/pflag"
)
func main() {
dynamicFlags := make(map[string]*string)
fmt.Println(os.Args)
flagSet := pflag.NewFlagSet("dynamicFlags", pflag.ExitOnError)
flagSet.AddGoFlagSet(flag.CommandLine)
justTheFlags := ExtractFlagNames(os.Args)
for _, flagName := range justTheFlags {
dynamicFlags[flagName] = flagSet.String(flagName, "", "")
}
if err := flagSet.Parse(os.Args); err != nil {
fmt.Println(err.Error())
return
}
for flagName, flagValue := range dynamicFlags {
fmt.Printf("%s: %s\n", flagName, *flagValue)
}
}
func ExtractFlagNames(args []string) (names []string) {
names = make([]string, 0)
for _, arg := range args {
if strings.HasPrefix(arg, "--param.") {
name, _ := strings.CutPrefix(arg, "--")
if i := strings.Index(name, "="); i > 0 {
name = name[:i]
}
names = append(names, name)
}
}
return
}
Then run with:
go run main.go command --param.J=50 --param.K "a string"
