From f2d5ed9933b417a4c4d5781c6aa4c1a2e73b6bbb Mon Sep 17 00:00:00 2001 From: Dan Buch Date: Wed, 18 May 2016 08:20:15 -0400 Subject: [PATCH 1/2] Replace BoolTFlag type with BoolFlag.Value Closes #412 --- CHANGELOG.md | 5 +-- README.md | 3 +- altsrc/flag.go | 38 ----------------- altsrc/flag_test.go | 30 ------------- altsrc/input_source_context.go | 1 - altsrc/map_input_source.go | 22 ---------- context.go | 21 ---------- context_test.go | 7 ---- flag.go | 55 +++--------------------- flag_test.go | 77 ---------------------------------- 10 files changed, 9 insertions(+), 250 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c94a17..a0c20a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - `NewStringSlice` and `NewIntSlice` for creating their related types - `Context.Lineage` to get all contexts from current up to global - `Context.LocalFlagNames` to get the flag names from *only* the current context +- `BoolFlag.Value` to handle both default-false and default-true ### Changed - `Context.FlagNames` now returns all flags in the context lineage @@ -28,9 +29,7 @@ - All `Context.Global*` methods, as the non-global versions now traverse up the context lineage automatically. - `Context.Parent` method, as this is now available via `Context.Lineage` - -### Fixed -- `Context.BoolT` now returns `true` when not found +- `BoolTFlag` and related code, as this is now available via `BoolFlag.Value` ## [Unreleased] - (1.x series) ### Added diff --git a/README.md b/README.md index a5bab32..fb218dc 100644 --- a/README.md +++ b/README.md @@ -472,8 +472,9 @@ import ( func main() { app := cli.NewApp() app.Flags = []cli.Flag{ - cli.BoolTFlag{ + cli.BoolFlag{ Name: "ginger-crouton", + Value: true, Usage: "is it in the soup?", }, } diff --git a/altsrc/flag.go b/altsrc/flag.go index d934e6a..aea142e 100644 --- a/altsrc/flag.go +++ b/altsrc/flag.go @@ -220,44 +220,6 @@ func (f *BoolFlag) Apply(set *flag.FlagSet) { f.BoolFlag.Apply(set) } -// BoolTFlag is the flag type that wraps cli.BoolTFlag to allow -// for other values to be specified -type BoolTFlag struct { - cli.BoolTFlag - set *flag.FlagSet -} - -// NewBoolTFlag creates a new BoolTFlag -func NewBoolTFlag(flag cli.BoolTFlag) *BoolTFlag { - return &BoolTFlag{BoolTFlag: flag, set: nil} -} - -// ApplyInputSourceValue applies a BoolT value to the flagSet if required -func (f *BoolTFlag) ApplyInputSourceValue(context *cli.Context, isc InputSourceContext) error { - if f.set != nil { - if !context.IsSet(f.Name) && !isEnvVarSet(f.EnvVar) { - value, err := isc.BoolT(f.BoolTFlag.Name) - if err != nil { - return err - } - if !value { - eachName(f.Name, func(name string) { - f.set.Set(f.Name, strconv.FormatBool(value)) - }) - } - } - } - return nil -} - -// Apply saves the flagSet for later usage then calls -// the wrapped BoolTFlag.Apply -func (f *BoolTFlag) Apply(set *flag.FlagSet) { - f.set = set - - f.BoolTFlag.Apply(set) -} - // StringFlag is the flag type that wraps cli.StringFlag to allow // for other values to be specified type StringFlag struct { diff --git a/altsrc/flag_test.go b/altsrc/flag_test.go index 4e25be6..be7166f 100644 --- a/altsrc/flag_test.go +++ b/altsrc/flag_test.go @@ -145,36 +145,6 @@ func TestBoolApplyInputSourceMethodEnvVarSet(t *testing.T) { expect(t, true, c.Bool("test")) } -func TestBoolTApplyInputSourceMethodSet(t *testing.T) { - c := runTest(t, testApplyInputSource{ - Flag: NewBoolTFlag(cli.BoolTFlag{Name: "test"}), - FlagName: "test", - MapValue: false, - }) - expect(t, false, c.BoolT("test")) -} - -func TestBoolTApplyInputSourceMethodContextSet(t *testing.T) { - c := runTest(t, testApplyInputSource{ - Flag: NewBoolTFlag(cli.BoolTFlag{Name: "test"}), - FlagName: "test", - MapValue: true, - ContextValueString: "false", - }) - expect(t, false, c.BoolT("test")) -} - -func TestBoolTApplyInputSourceMethodEnvVarSet(t *testing.T) { - c := runTest(t, testApplyInputSource{ - Flag: NewBoolTFlag(cli.BoolTFlag{Name: "test", EnvVar: "TEST"}), - FlagName: "test", - MapValue: true, - EnvVarName: "TEST", - EnvVarValue: "false", - }) - expect(t, false, c.BoolT("test")) -} - func TestStringApplyInputSourceMethodSet(t *testing.T) { c := runTest(t, testApplyInputSource{ Flag: NewStringFlag(cli.StringFlag{Name: "test"}), diff --git a/altsrc/input_source_context.go b/altsrc/input_source_context.go index 6d695ff..0f391b2 100644 --- a/altsrc/input_source_context.go +++ b/altsrc/input_source_context.go @@ -17,5 +17,4 @@ type InputSourceContext interface { IntSlice(name string) ([]int, error) Generic(name string) (cli.Generic, error) Bool(name string) (bool, error) - BoolT(name string) (bool, error) } diff --git a/altsrc/map_input_source.go b/altsrc/map_input_source.go index 19f87af..a7fc628 100644 --- a/altsrc/map_input_source.go +++ b/altsrc/map_input_source.go @@ -215,28 +215,6 @@ func (fsm *MapInputSource) Bool(name string) (bool, error) { return false, nil } -// BoolT returns an bool from the map otherwise returns true -func (fsm *MapInputSource) BoolT(name string) (bool, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - otherValue, isType := otherGenericValue.(bool) - if !isType { - return true, incorrectTypeForFlagError(name, "bool", otherGenericValue) - } - return otherValue, nil - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - otherValue, isType := nestedGenericValue.(bool) - if !isType { - return true, incorrectTypeForFlagError(name, "bool", nestedGenericValue) - } - return otherValue, nil - } - - return true, nil -} - func incorrectTypeForFlagError(name, expectedTypeName string, value interface{}) error { valueType := reflect.TypeOf(value) valueTypeName := "" diff --git a/context.go b/context.go index 03c61b2..45face1 100644 --- a/context.go +++ b/context.go @@ -59,14 +59,6 @@ func (c *Context) Bool(name string) bool { return false } -// BoolT looks up the value of a local boolT flag, returns false if no bool flag exists -func (c *Context) BoolT(name string) bool { - if fs := lookupFlagSet(name, c); fs != nil { - return lookupBoolT(name, fs) - } - return true -} - // String looks up the value of a local string flag, returns "" if no string flag exists func (c *Context) String(name string) string { if fs := lookupFlagSet(name, c); fs != nil { @@ -303,19 +295,6 @@ func lookupBool(name string, set *flag.FlagSet) bool { return false } -func lookupBoolT(name string, set *flag.FlagSet) bool { - f := set.Lookup(name) - if f != nil { - val, err := strconv.ParseBool(f.Value.String()) - if err != nil { - return true - } - return val - } - - return true -} - func copyFlag(name string, ff *flag.Flag, set *flag.FlagSet) { switch ff.Value.(type) { case Serializeder: diff --git a/context_test.go b/context_test.go index bdb77fd..8147965 100644 --- a/context_test.go +++ b/context_test.go @@ -58,13 +58,6 @@ func TestContext_Bool(t *testing.T) { expect(t, c.Bool("myflag"), false) } -func TestContext_BoolT(t *testing.T) { - set := flag.NewFlagSet("test", 0) - set.Bool("myflag", true, "doc") - c := NewContext(nil, set, nil) - expect(t, c.BoolT("myflag"), true) -} - func TestContext_Args(t *testing.T) { set := flag.NewFlagSet("test", 0) set.Bool("myflag", false, "doc") diff --git a/flag.go b/flag.go index 5694142..97e502c 100644 --- a/flag.go +++ b/flag.go @@ -321,6 +321,7 @@ func (f IntSliceFlag) GetName() string { // BoolFlag is a switch that defaults to false type BoolFlag struct { Name string + Value bool Usage string EnvVar string Destination *bool @@ -334,14 +335,13 @@ func (f BoolFlag) String() string { // Apply populates the flag given the flag set and environment func (f BoolFlag) Apply(set *flag.FlagSet) { - val := false if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal := os.Getenv(envVar); envVal != "" { envValBool, err := strconv.ParseBool(envVal) if err == nil { - val = envValBool + f.Value = envValBool } break } @@ -350,10 +350,10 @@ func (f BoolFlag) Apply(set *flag.FlagSet) { eachName(f.Name, func(name string) { if f.Destination != nil { - set.BoolVar(f.Destination, name, val, f.Usage) + set.BoolVar(f.Destination, name, f.Value, f.Usage) return } - set.Bool(name, val, f.Usage) + set.Bool(name, f.Value, f.Usage) }) } @@ -362,51 +362,6 @@ func (f BoolFlag) GetName() string { return f.Name } -// BoolTFlag this represents a boolean flag that is true by default, but can -// still be set to false by --some-flag=false -type BoolTFlag struct { - Name string - Usage string - EnvVar string - Destination *bool - Hidden bool -} - -// String returns a readable representation of this value (for usage defaults) -func (f BoolTFlag) String() string { - return FlagStringer(f) -} - -// Apply populates the flag given the flag set and environment -func (f BoolTFlag) Apply(set *flag.FlagSet) { - val := true - if f.EnvVar != "" { - for _, envVar := range strings.Split(f.EnvVar, ",") { - envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { - envValBool, err := strconv.ParseBool(envVal) - if err == nil { - val = envValBool - break - } - } - } - } - - eachName(f.Name, func(name string) { - if f.Destination != nil { - set.BoolVar(f.Destination, name, val, f.Usage) - return - } - set.Bool(name, val, f.Usage) - }) -} - -// GetName returns the name of the flag. -func (f BoolTFlag) GetName() string { - return f.Name -} - // StringFlag represents a flag that takes as string value type StringFlag struct { Name string @@ -669,7 +624,7 @@ func stringifyFlag(f Flag) string { defaultValueString := "" val := fv.FieldByName("Value") - if val.IsValid() { + if val.IsValid() && val.Kind() != reflect.Bool { needsPlaceholder = true defaultValueString = fmt.Sprintf(" (default: %v)", val.Interface()) diff --git a/flag_test.go b/flag_test.go index f163468..3cf6833 100644 --- a/flag_test.go +++ b/flag_test.go @@ -843,83 +843,6 @@ func TestParseMultiBoolFromEnvCascade(t *testing.T) { a.Run([]string{"run"}) } -func TestParseMultiBoolT(t *testing.T) { - a := App{ - Flags: []Flag{ - BoolTFlag{Name: "serve, s"}, - }, - Action: func(ctx *Context) error { - if ctx.BoolT("serve") != true { - t.Errorf("main name not set") - } - if ctx.BoolT("s") != true { - t.Errorf("short name not set") - } - return nil - }, - } - a.Run([]string{"run", "--serve"}) -} - -func TestParseDestinationBoolT(t *testing.T) { - var dest bool - a := App{ - Flags: []Flag{ - BoolTFlag{ - Name: "dest", - Destination: &dest, - }, - }, - Action: func(ctx *Context) error { - if dest != true { - t.Errorf("expected destination BoolT true") - } - return nil - }, - } - a.Run([]string{"run", "--dest"}) -} - -func TestParseMultiBoolTFromEnv(t *testing.T) { - os.Clearenv() - os.Setenv("APP_DEBUG", "0") - a := App{ - Flags: []Flag{ - BoolTFlag{Name: "debug, d", EnvVar: "APP_DEBUG"}, - }, - Action: func(ctx *Context) error { - if ctx.BoolT("debug") != false { - t.Errorf("main name not set from env") - } - if ctx.BoolT("d") != false { - t.Errorf("short name not set from env") - } - return nil - }, - } - a.Run([]string{"run"}) -} - -func TestParseMultiBoolTFromEnvCascade(t *testing.T) { - os.Clearenv() - os.Setenv("APP_DEBUG", "0") - a := App{ - Flags: []Flag{ - BoolTFlag{Name: "debug, d", EnvVar: "COMPAT_DEBUG,APP_DEBUG"}, - }, - Action: func(ctx *Context) error { - if ctx.BoolT("debug") != false { - t.Errorf("main name not set from env") - } - if ctx.BoolT("d") != false { - t.Errorf("short name not set from env") - } - return nil - }, - } - a.Run([]string{"run"}) -} - type Parser [2]string func (p *Parser) Set(value string) error { From 9c132d990a17351e6274ae24eab7e07752305194 Mon Sep 17 00:00:00 2001 From: Dan Buch Date: Wed, 18 May 2016 08:30:28 -0400 Subject: [PATCH 2/2] Add more tests for BoolFlag{Value: true} --- flag_test.go | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/flag_test.go b/flag_test.go index 3cf6833..a3414cf 100644 --- a/flag_test.go +++ b/flag_test.go @@ -843,6 +843,93 @@ func TestParseMultiBoolFromEnvCascade(t *testing.T) { a.Run([]string{"run"}) } +func TestParseMultiBoolTrue(t *testing.T) { + a := App{ + Flags: []Flag{ + BoolFlag{Name: "implode, i", Value: true}, + }, + Action: func(ctx *Context) error { + if ctx.Bool("implode") { + t.Errorf("main name not set") + } + if ctx.Bool("i") { + t.Errorf("short name not set") + } + return nil + }, + } + a.Run([]string{"run", "--implode=false"}) +} + +func TestParseDestinationBoolTrue(t *testing.T) { + dest := true + + a := App{ + Flags: []Flag{ + BoolFlag{ + Name: "dest", + Value: true, + Destination: &dest, + }, + }, + Action: func(ctx *Context) error { + if dest { + t.Errorf("expected destination Bool false") + } + return nil + }, + } + a.Run([]string{"run", "--dest=false"}) +} + +func TestParseMultiBoolTrueFromEnv(t *testing.T) { + os.Clearenv() + os.Setenv("APP_DEBUG", "0") + a := App{ + Flags: []Flag{ + BoolFlag{ + Name: "debug, d", + Value: true, + EnvVar: "APP_DEBUG", + }, + }, + Action: func(ctx *Context) error { + if ctx.Bool("debug") { + t.Errorf("main name not set from env") + } + if ctx.Bool("d") { + t.Errorf("short name not set from env") + } + return nil + }, + } + a.Run([]string{"run"}) +} + +func TestParseMultiBoolTrueFromEnvCascade(t *testing.T) { + os.Clearenv() + os.Setenv("APP_DEBUG", "0") + a := App{ + Flags: []Flag{ + BoolFlag{ + Name: "debug, d", + Value: true, + EnvVar: "COMPAT_DEBUG,APP_DEBUG", + }, + }, + Action: func(ctx *Context) error { + if ctx.Bool("debug") { + t.Errorf("main name not set from env") + } + if ctx.Bool("d") { + t.Errorf("short name not set from env") + } + return nil + }, + } + a.Run([]string{"run"}) +} + type Parser [2]string func (p *Parser) Set(value string) error {