From fc7b194f09fab61349a5914c413453db1bcce583 Mon Sep 17 00:00:00 2001 From: Naveen Gogineni Date: Sat, 26 Nov 2022 07:58:15 -0500 Subject: [PATCH 1/2] Rebase of generics changes --- app_test.go | 132 ++++++++++++++++++++++++++++++++++++++++++++++ command.go | 41 +++++++++++--- flag.go | 6 +++ flag_impl.go | 58 ++++++++++++-------- godoc-current.txt | 15 +++++- 5 files changed, 222 insertions(+), 30 deletions(-) diff --git a/app_test.go b/app_test.go index b6cd247a2e..dae95ca308 100644 --- a/app_test.go +++ b/app_test.go @@ -1461,6 +1461,10 @@ func TestApp_AfterFunc(t *testing.T) { reset */ counts = &opCounts{} + // reset the flags since they are set previously + app.Flags = []Flag{ + &StringFlag{Name: "opt"}, + } // run with none args err = app.Run([]string{"command"}) @@ -3053,3 +3057,131 @@ func TestFlagAction(t *testing.T) { }) } } + +func TestPersistentFlag(t *testing.T) { + + var topInt, topPersistentInt, subCommandInt, appOverrideInt int + var appFlag string + var appOverrideCmdInt int64 + var appSliceFloat64 []float64 + var persistentAppSliceInt []int64 + + a := &App{ + Flags: []Flag{ + &StringFlag{ + Name: "persistentAppFlag", + Persistent: true, + Destination: &appFlag, + }, + &Int64SliceFlag{ + Name: "persistentAppSliceFlag", + Persistent: true, + Destination: &persistentAppSliceInt, + }, + &Float64SliceFlag{ + Name: "persistentAppFloatSliceFlag", + Persistent: true, + Value: []float64{11.3, 12.5}, + }, + &IntFlag{ + Name: "persistentAppOverrideFlag", + Persistent: true, + Destination: &appOverrideInt, + }, + }, + Commands: []*Command{ + { + Name: "cmd", + Flags: []Flag{ + &IntFlag{ + Name: "cmdFlag", + Destination: &topInt, + }, + &IntFlag{ + Name: "cmdPersistentFlag", + Persistent: true, + Destination: &topPersistentInt, + }, + &Int64Flag{ + Name: "paof", + Aliases: []string{"persistentAppOverrideFlag"}, + Destination: &appOverrideCmdInt, + }, + }, + Subcommands: []*Command{ + { + Name: "subcmd", + Flags: []Flag{ + &IntFlag{ + Name: "cmdFlag", + Destination: &subCommandInt, + }, + }, + Action: func(ctx *Context) error { + appSliceFloat64 = ctx.Float64Slice("persistentAppFloatSliceFlag") + return nil + }, + }, + }, + }, + }, + } + + err := a.Run([]string{"app", + "--persistentAppFlag", "hello", + "--persistentAppSliceFlag", "100", + "--persistentAppOverrideFlag", "102", + "cmd", + "--cmdFlag", "12", + "--persistentAppSliceFlag", "102", + "--persistentAppFloatSliceFlag", "102.455", + "--paof", "105", + "subcmd", + "--cmdPersistentFlag", "20", + "--cmdFlag", "11", + "--persistentAppFlag", "bar", + "--persistentAppSliceFlag", "130", + "--persistentAppFloatSliceFlag", "3.1445", + }) + + if err != nil { + t.Fatal(err) + } + + if appFlag != "bar" { + t.Errorf("Expected 'bar' got %s", appFlag) + } + + if topInt != 12 { + t.Errorf("Expected 12 got %d", topInt) + } + + if topPersistentInt != 20 { + t.Errorf("Expected 20 got %d", topPersistentInt) + } + + // this should be changed from app since + // cmd overrides it + if appOverrideInt != 102 { + t.Errorf("Expected 102 got %d", appOverrideInt) + } + + if subCommandInt != 11 { + t.Errorf("Expected 11 got %d", subCommandInt) + } + + if appOverrideCmdInt != 105 { + t.Errorf("Expected 105 got %d", appOverrideCmdInt) + } + + expectedInt := []int64{100, 102, 130} + if !reflect.DeepEqual(persistentAppSliceInt, expectedInt) { + t.Errorf("Expected %v got %d", expectedInt, persistentAppSliceInt) + } + + expectedFloat := []float64{102.455, 3.1445} + if !reflect.DeepEqual(appSliceFloat64, expectedFloat) { + t.Errorf("Expected %f got %f", expectedFloat, appSliceFloat64) + } + +} diff --git a/command.go b/command.go index 04aab4f4ac..f648b922f1 100644 --- a/command.go +++ b/command.go @@ -146,7 +146,7 @@ func (c *Command) Run(cCtx *Context, arguments ...string) (err error) { } a := args(arguments) - set, err := c.parseFlags(&a, cCtx.shellComplete) + set, err := c.parseFlags(&a, cCtx) cCtx.flagSet = set if c.isRoot { @@ -308,7 +308,7 @@ func (c *Command) suggestFlagFromError(err error, command string) (string, error return fmt.Sprintf(SuggestDidYouMeanTemplate, suggestion) + "\n\n", nil } -func (c *Command) parseFlags(args Args, shellComplete bool) (*flag.FlagSet, error) { +func (c *Command) parseFlags(args Args, ctx *Context) (*flag.FlagSet, error) { set, err := c.newFlagSet() if err != nil { return nil, err @@ -318,13 +318,42 @@ func (c *Command) parseFlags(args Args, shellComplete bool) (*flag.FlagSet, erro return set, set.Parse(append([]string{"--"}, args.Tail()...)) } - err = parseIter(set, c, args.Tail(), shellComplete) - if err != nil { + for pCtx := ctx.parentContext; pCtx != nil; pCtx = pCtx.parentContext { + if pCtx.Command == nil { + continue + } + + for _, fl := range pCtx.Command.Flags { + pfl, ok := fl.(PersistentFlag) + if !ok || !pfl.IsPersistent() { + continue + } + + applyPersistentFlag := true + set.VisitAll(func(f *flag.Flag) { + for _, name := range fl.Names() { + if name == f.Name { + applyPersistentFlag = false + break + } + } + }) + + if !applyPersistentFlag { + continue + } + + if err := fl.Apply(set); err != nil { + return nil, err + } + } + } + + if err := parseIter(set, c, args.Tail(), ctx.shellComplete); err != nil { return nil, err } - err = normalizeFlags(c.Flags, set) - if err != nil { + if err := normalizeFlags(c.Flags, set); err != nil { return nil, err } diff --git a/flag.go b/flag.go index 7970e8d39c..4d0b122750 100644 --- a/flag.go +++ b/flag.go @@ -172,6 +172,12 @@ type CategorizableFlag interface { GetCategory() string } +// PersistentFlag is an interface to enable detection of flags which are persistent +// through subcommands +type PersistentFlag interface { + IsPersistent() bool +} + func flagSet(name string, flags []Flag) (*flag.FlagSet, error) { set := flag.NewFlagSet(name, flag.ContinueOnError) diff --git a/flag_impl.go b/flag_impl.go index 89b04f81c3..5c54beb47c 100644 --- a/flag_impl.go +++ b/flag_impl.go @@ -41,8 +41,9 @@ type FlagBase[T any, C any, VC ValueCreator[T, C]] struct { FilePath string // file path to load value from Usage string // usage string for help output - Required bool // whether the flag is required or not - Hidden bool // whether to hide the flag in help output + Required bool // whether the flag is required or not + Hidden bool // whether to hide the flag in help output + Persistent bool // whether the flag needs to be applied to subcommands as well Value T // default value for this flag if not set by from any source Destination *T // destination pointer for value when set @@ -58,6 +59,7 @@ type FlagBase[T any, C any, VC ValueCreator[T, C]] struct { // unexported fields for internal use hasBeenSet bool // whether the flag has been set from env or file + applied bool // whether the flag has been applied to a flag set already creator VC // value creator for this flag type value Value // value representing this flag's value } @@ -73,35 +75,41 @@ func (f *FlagBase[T, C, V]) GetValue() string { // Apply populates the flag given the flag set and environment func (f *FlagBase[T, C, V]) Apply(set *flag.FlagSet) error { - newVal := f.Value - - if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found { - tmpVal := f.creator.Create(f.Value, new(T), f.Config) - if val != "" || reflect.TypeOf(f.Value).Kind() == reflect.String { - if err := tmpVal.Set(val); err != nil { - return fmt.Errorf("could not parse %q as %T value from %s for flag %s: %s", val, f.Value, source, f.Name, err) - } - } else if val == "" && reflect.TypeOf(f.Value).Kind() == reflect.Bool { - val = "false" - if err := tmpVal.Set(val); err != nil { - return fmt.Errorf("could not parse %q as %T value from %s for flag %s: %s", val, f.Value, source, f.Name, err) + // if flag has been applied then it wouldnt have been set from + // env or file as well. + // TODO move this phase into a separate flag initialization function + if !f.applied { + newVal := f.Value + + if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found { + tmpVal := f.creator.Create(f.Value, new(T), f.Config) + if val != "" || reflect.TypeOf(f.Value).Kind() == reflect.String { + if err := tmpVal.Set(val); err != nil { + return fmt.Errorf("could not parse %q as %T value from %s for flag %s: %s", val, f.Value, source, f.Name, err) + } + } else if val == "" && reflect.TypeOf(f.Value).Kind() == reflect.Bool { + val = "false" + if err := tmpVal.Set(val); err != nil { + return fmt.Errorf("could not parse %q as %T value from %s for flag %s: %s", val, f.Value, source, f.Name, err) + } } - } - newVal = tmpVal.Get().(T) - f.hasBeenSet = true - } + newVal = tmpVal.Get().(T) + f.hasBeenSet = true + } - if f.Destination == nil { - f.value = f.creator.Create(newVal, new(T), f.Config) - } else { - f.value = f.creator.Create(newVal, f.Destination, f.Config) + if f.Destination == nil { + f.value = f.creator.Create(newVal, new(T), f.Config) + } else { + f.value = f.creator.Create(newVal, f.Destination, f.Config) + } } for _, name := range f.Names() { set.Var(f.value, name, f.Usage) } + f.applied = true return nil } @@ -178,7 +186,13 @@ func (f *FlagBase[T, C, V]) RunAction(ctx *Context) error { return nil } +// IsSliceFlag returns true if the value type T is of kind slice func (f *FlagBase[T, C, VC]) IsSliceFlag() bool { // TBD how to specify return reflect.TypeOf(f.Value).Kind() == reflect.Slice } + +// IsPersistent returns true if flag needs to be persistent across subcommands +func (f *FlagBase[T, C, VC]) IsPersistent() bool { + return f.Persistent +} diff --git a/godoc-current.txt b/godoc-current.txt index ebdb9917b5..4f426b7ace 100644 --- a/godoc-current.txt +++ b/godoc-current.txt @@ -780,8 +780,9 @@ type FlagBase[T any, C any, VC ValueCreator[T, C]] struct { FilePath string // file path to load value from Usage string // usage string for help output - Required bool // whether the flag is required or not - Hidden bool // whether to hide the flag in help output + Required bool // whether the flag is required or not + Hidden bool // whether to hide the flag in help output + Persistent bool // whether the flag needs to be applied to subcommands as well Value T // default value for this flag if not set by from any source Destination *T // destination pointer for value when set @@ -826,6 +827,9 @@ func (f *FlagBase[T, C, V]) GetValue() string GetValue returns the flags value as string representation and an empty string if the flag takes no value at all. +func (f *FlagBase[T, C, VC]) IsPersistent() bool + IsPersistent returns true if flag needs to be persistent across subcommands + func (f *FlagBase[T, C, V]) IsRequired() bool IsRequired returns whether or not the flag is required @@ -833,6 +837,7 @@ func (f *FlagBase[T, C, V]) IsSet() bool IsSet returns whether or not the flag has been set through env or file func (f *FlagBase[T, C, VC]) IsSliceFlag() bool + IsSliceFlag returns true if the value type T is of kind slice func (f *FlagBase[T, C, V]) IsVisible() bool IsVisible returns true if the flag is not hidden, otherwise false @@ -940,6 +945,12 @@ type OnUsageErrorFunc func(cCtx *Context, err error, isSubcommand bool) error the original error messages. If this function is not set, the "Incorrect usage" is displayed and the execution is interrupted. +type PersistentFlag interface { + IsPersistent() bool +} + PersistentFlag is an interface to enable detection of flags which are + persistent through subcommands + type RequiredFlag interface { Flag From e0d58176a627b1f9ca0585ebb5e8d82a2ba63b63 Mon Sep 17 00:00:00 2001 From: Naveen Gogineni Date: Sat, 26 Nov 2022 20:19:11 -0500 Subject: [PATCH 2/2] Fix test failures --- flag_impl.go | 9 ++++++--- testdata/godoc-v3.x.txt | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/flag_impl.go b/flag_impl.go index 5c54beb47c..cd3d3a5e14 100644 --- a/flag_impl.go +++ b/flag_impl.go @@ -75,10 +75,13 @@ func (f *FlagBase[T, C, V]) GetValue() string { // Apply populates the flag given the flag set and environment func (f *FlagBase[T, C, V]) Apply(set *flag.FlagSet) error { - // if flag has been applied then it wouldnt have been set from - // env or file as well. // TODO move this phase into a separate flag initialization function - if !f.applied { + // if flag has been applied previously then it would have already been set + // from env or file. So no need to apply the env set again. However + // lots of units tests prior to persistent flags assumed that the + // flag can be applied to different flag sets multiple times while still + // keeping the env set. + if !f.applied || !f.Persistent { newVal := f.Value if val, source, found := flagFromEnvOrFile(f.EnvVars, f.FilePath); found { diff --git a/testdata/godoc-v3.x.txt b/testdata/godoc-v3.x.txt index ebdb9917b5..4f426b7ace 100644 --- a/testdata/godoc-v3.x.txt +++ b/testdata/godoc-v3.x.txt @@ -780,8 +780,9 @@ type FlagBase[T any, C any, VC ValueCreator[T, C]] struct { FilePath string // file path to load value from Usage string // usage string for help output - Required bool // whether the flag is required or not - Hidden bool // whether to hide the flag in help output + Required bool // whether the flag is required or not + Hidden bool // whether to hide the flag in help output + Persistent bool // whether the flag needs to be applied to subcommands as well Value T // default value for this flag if not set by from any source Destination *T // destination pointer for value when set @@ -826,6 +827,9 @@ func (f *FlagBase[T, C, V]) GetValue() string GetValue returns the flags value as string representation and an empty string if the flag takes no value at all. +func (f *FlagBase[T, C, VC]) IsPersistent() bool + IsPersistent returns true if flag needs to be persistent across subcommands + func (f *FlagBase[T, C, V]) IsRequired() bool IsRequired returns whether or not the flag is required @@ -833,6 +837,7 @@ func (f *FlagBase[T, C, V]) IsSet() bool IsSet returns whether or not the flag has been set through env or file func (f *FlagBase[T, C, VC]) IsSliceFlag() bool + IsSliceFlag returns true if the value type T is of kind slice func (f *FlagBase[T, C, V]) IsVisible() bool IsVisible returns true if the flag is not hidden, otherwise false @@ -940,6 +945,12 @@ type OnUsageErrorFunc func(cCtx *Context, err error, isSubcommand bool) error the original error messages. If this function is not set, the "Incorrect usage" is displayed and the execution is interrupted. +type PersistentFlag interface { + IsPersistent() bool +} + PersistentFlag is an interface to enable detection of flags which are + persistent through subcommands + type RequiredFlag interface { Flag