From 79591889a97af09f177601e3e9e61f9c7e359359 Mon Sep 17 00:00:00 2001 From: mh-cbon Date: Sat, 5 Nov 2016 10:27:46 +0100 Subject: [PATCH 01/10] Close #558: detect FormattedError and print their stack trace --- errors.go | 14 ++++++++++---- errors_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/errors.go b/errors.go index ddef369..e0f295e 100644 --- a/errors.go +++ b/errors.go @@ -34,6 +34,10 @@ func (m MultiError) Error() string { return strings.Join(errs, "\n") } +type ErrorFormatter interface { + Format(s fmt.State, verb rune) +} + // ExitCoder is the interface checked by `App` and `Command` for a custom exit // code type ExitCoder interface { @@ -44,11 +48,11 @@ type ExitCoder interface { // ExitError fulfills both the builtin `error` interface and `ExitCoder` type ExitError struct { exitCode int - message string + message interface{} } // NewExitError makes a new *ExitError -func NewExitError(message string, exitCode int) *ExitError { +func NewExitError(message interface{}, exitCode int) *ExitError { return &ExitError{ exitCode: exitCode, message: message, @@ -58,7 +62,7 @@ func NewExitError(message string, exitCode int) *ExitError { // Error returns the string message, fulfilling the interface required by // `error` func (ee *ExitError) Error() string { - return ee.message + return fmt.Sprintf("%v", ee.message) } // ExitCode returns the exit code, fulfilling the interface required by @@ -77,7 +81,9 @@ func HandleExitCoder(err error) { } if exitErr, ok := err.(ExitCoder); ok { - if err.Error() != "" { + if _, ok := exitErr.(ErrorFormatter); ok { + fmt.Fprintf(ErrWriter, "%+v\n", err) + } else { fmt.Fprintln(ErrWriter, err) } OsExiter(exitErr.ExitCode()) diff --git a/errors_test.go b/errors_test.go index 04df031..e6dfc16 100644 --- a/errors_test.go +++ b/errors_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "errors" + "fmt" "testing" ) @@ -104,3 +105,32 @@ func TestHandleExitCoder_ErrorWithoutMessage(t *testing.T) { expect(t, called, true) expect(t, ErrWriter.(*bytes.Buffer).String(), "") } + +// make a stub to not import pkg/errors +type ErrorWithFormat struct { + error +} + +func (f *ErrorWithFormat) Format(s fmt.State, verb rune) { + fmt.Fprintf(s, "This the format: %v", f.error) +} + +func TestHandleExitCoder_ErrorWithFormat(t *testing.T) { + called := false + + OsExiter = func(rc int) { + called = true + } + ErrWriter = &bytes.Buffer{} + + defer func() { + OsExiter = fakeOsExiter + ErrWriter = fakeErrWriter + }() + + err := &ErrorWithFormat{error: errors.New("I am formatted")} + HandleExitCoder(err) + + expect(t, called, true) + expect(t, ErrWriter.(*bytes.Buffer).String(), "This the format: I am formatted\n") +} From 0113f56d1094e8565f6d1ad10526865fd61d57aa Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 12 Nov 2016 13:37:07 -0800 Subject: [PATCH 02/10] If no action is specified on the command or app, print the help documentation Rather than panic'ing or displaying an opaque error message about the signature which is more confusing to the end user. Fixes #562 --- app.go | 4 ++++ app_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ command.go | 4 ++++ help.go | 2 +- 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/app.go b/app.go index 26cf09a..a90a4df 100644 --- a/app.go +++ b/app.go @@ -242,6 +242,10 @@ func (a *App) Run(arguments []string) (err error) { } } + if a.Action == nil { + a.Action = helpCommand.Action + } + // Run default Action err = HandleAction(a.Action, context) diff --git a/app_test.go b/app_test.go index 40a598d..711de47 100644 --- a/app_test.go +++ b/app_test.go @@ -178,6 +178,49 @@ func ExampleApp_Run_commandHelp() { // This is how we describe describeit the function } +func ExampleApp_Run_noAction() { + app := App{} + app.Name = "greet" + app.Run([]string{"greet"}) + // Output: + // NAME: + // greet + // + // USAGE: + // [global options] command [command options] [arguments...] + // + // COMMANDS: + // help, h Shows a list of commands or help for one command + // + // GLOBAL OPTIONS: + // --help, -h show help + // --version, -v print the version +} + +func ExampleApp_Run_subcommandNoAction() { + app := App{} + app.Name = "greet" + app.Commands = []Command{ + { + Name: "describeit", + Aliases: []string{"d"}, + Usage: "use it to see a description", + Description: "This is how we describe describeit the function", + }, + } + app.Run([]string{"greet", "describeit"}) + // Output: + // NAME: + // describeit - use it to see a description + // + // USAGE: + // describeit [arguments...] + // + // DESCRIPTION: + // This is how we describe describeit the function + +} + func ExampleApp_Run_bashComplete() { // set args for examples sake os.Args = []string{"greet", "--generate-bash-completion"} diff --git a/command.go b/command.go index d955249..8f1a215 100644 --- a/command.go +++ b/command.go @@ -187,6 +187,10 @@ func (c Command) Run(ctx *Context) (err error) { } } + if c.Action == nil { + c.Action = helpSubcommand.Action + } + context.Command = c err = HandleAction(c.Action, context) diff --git a/help.go b/help.go index 515f744..5ee11a6 100644 --- a/help.go +++ b/help.go @@ -13,7 +13,7 @@ import ( // cli.go uses text/template to render templates. You can // render custom help text by setting this variable. var AppHelpTemplate = `NAME: - {{.Name}} - {{.Usage}} + {{.Name}}{{if .Usage}} - {{.Usage}}{{end}} USAGE: {{if .UsageText}}{{.UsageText}}{{else}}{{.HelpName}} {{if .VisibleFlags}}[global options]{{end}}{{if .Commands}} command [command options]{{end}} {{if .ArgsUsage}}{{.ArgsUsage}}{{else}}[arguments...]{{end}}{{end}}{{if .Version}}{{if not .HideVersion}} From b0a8f25773c10b7445d0d740d380a56e3dcd2166 Mon Sep 17 00:00:00 2001 From: mh-cbon Date: Sun, 13 Nov 2016 22:20:13 +0100 Subject: [PATCH 03/10] 558: handle multi formatter errors --- errors.go | 16 +++++++++++----- errors_test.go | 23 ++++++++++++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/errors.go b/errors.go index e0f295e..0206ff4 100644 --- a/errors.go +++ b/errors.go @@ -81,10 +81,12 @@ func HandleExitCoder(err error) { } if exitErr, ok := err.(ExitCoder); ok { - if _, ok := exitErr.(ErrorFormatter); ok { - fmt.Fprintf(ErrWriter, "%+v\n", err) - } else { - fmt.Fprintln(ErrWriter, err) + if err.Error() != "" { + if _, ok := exitErr.(ErrorFormatter); ok { + fmt.Fprintf(ErrWriter, "%+v\n", err) + } else { + fmt.Fprintln(ErrWriter, err) + } } OsExiter(exitErr.ExitCode()) return @@ -98,7 +100,11 @@ func HandleExitCoder(err error) { } if err.Error() != "" { - fmt.Fprintln(ErrWriter, err) + if _, ok := err.(ErrorFormatter); ok { + fmt.Fprintf(ErrWriter, "%+v\n", err) + } else { + fmt.Fprintln(ErrWriter, err) + } } OsExiter(1) } diff --git a/errors_test.go b/errors_test.go index e6dfc16..131bd38 100644 --- a/errors_test.go +++ b/errors_test.go @@ -111,6 +111,10 @@ type ErrorWithFormat struct { error } +func NewErrorWithFormat(m string) *ErrorWithFormat { + return &ErrorWithFormat{error: errors.New(m)} +} + func (f *ErrorWithFormat) Format(s fmt.State, verb rune) { fmt.Fprintf(s, "This the format: %v", f.error) } @@ -128,9 +132,26 @@ func TestHandleExitCoder_ErrorWithFormat(t *testing.T) { ErrWriter = fakeErrWriter }() - err := &ErrorWithFormat{error: errors.New("I am formatted")} + err := NewErrorWithFormat("I am formatted") HandleExitCoder(err) expect(t, called, true) expect(t, ErrWriter.(*bytes.Buffer).String(), "This the format: I am formatted\n") } + +func TestHandleExitCoder_MultiErrorWithFormat(t *testing.T) { + called := false + + OsExiter = func(rc int) { + called = true + } + ErrWriter = &bytes.Buffer{} + + defer func() { OsExiter = fakeOsExiter }() + + err := NewMultiError(NewErrorWithFormat("err1"), NewErrorWithFormat("err2")) + HandleExitCoder(err) + + expect(t, called, true) + expect(t, ErrWriter.(*bytes.Buffer).String(), "This the format: err1\nThis the format: err2\n") +} From a00c3f5872b3129785af69ee16a2912cd58079d3 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sun, 11 Sep 2016 20:32:43 -0700 Subject: [PATCH 04/10] Consider empty environment variables as set When assigning values to flags (also when interogatting via `context.(Global)IsSet`. For boolean flags, consider empty as `false`. Using `syscall.Getenv` rather than `os.LookupEnv` in order to support older Golang versions. --- altsrc/flag.go | 8 +++---- context.go | 4 ++-- context_test.go | 8 ++++++- flag.go | 38 ++++++++++++++++++----------- flag_test.go | 64 +++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 22 deletions(-) diff --git a/altsrc/flag.go b/altsrc/flag.go index ec14e40..84ef009 100644 --- a/altsrc/flag.go +++ b/altsrc/flag.go @@ -2,9 +2,9 @@ package altsrc import ( "fmt" - "os" "strconv" "strings" + "syscall" "gopkg.in/urfave/cli.v1" ) @@ -237,13 +237,11 @@ func (f *Float64Flag) ApplyInputSourceValue(context *cli.Context, isc InputSourc func isEnvVarSet(envVars string) bool { for _, envVar := range strings.Split(envVars, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if _, ok := syscall.Getenv(envVar); ok { // TODO: Can't use this for bools as // set means that it was true or false based on // Bool flag type, should work for other types - if len(envVal) > 0 { - return true - } + return true } } diff --git a/context.go b/context.go index 492a742..3893a50 100644 --- a/context.go +++ b/context.go @@ -3,9 +3,9 @@ package cli import ( "errors" "flag" - "os" "reflect" "strings" + "syscall" ) // Context is a type that is passed through to @@ -91,7 +91,7 @@ func (c *Context) IsSet(name string) bool { eachName(envVarValue.String(), func(envVar string) { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if _, ok := syscall.Getenv(envVar); ok { c.setFlags[name] = true return } diff --git a/context_test.go b/context_test.go index 0cf84d1..b5bc993 100644 --- a/context_test.go +++ b/context_test.go @@ -184,18 +184,22 @@ func TestContext_IsSet(t *testing.T) { // XXX Corresponds to hack in context.IsSet for flags with EnvVar field // Should be moved to `flag_test` in v2 func TestContext_IsSet_fromEnv(t *testing.T) { - var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool + var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet, passwordIsSet, pIsSet bool os.Clearenv() os.Setenv("APP_TIMEOUT_SECONDS", "15.5") + os.Setenv("APP_PASSWORD", "") a := App{ Flags: []Flag{ Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"}, + Float64Flag{Name: "password, p", EnvVar: "APP_PASSWORD"}, Float64Flag{Name: "no-env-var, n"}, }, Action: func(ctx *Context) error { timeoutIsSet = ctx.IsSet("timeout") tIsSet = ctx.IsSet("t") + passwordIsSet = ctx.IsSet("password") + pIsSet = ctx.IsSet("p") noEnvVarIsSet = ctx.IsSet("no-env-var") nIsSet = ctx.IsSet("n") return nil @@ -204,6 +208,8 @@ func TestContext_IsSet_fromEnv(t *testing.T) { a.Run([]string{"run"}) expect(t, timeoutIsSet, true) expect(t, tIsSet, true) + expect(t, passwordIsSet, true) + expect(t, pIsSet, true) expect(t, noEnvVarIsSet, false) expect(t, nIsSet, false) } diff --git a/flag.go b/flag.go index 1ff28d3..5e565fb 100644 --- a/flag.go +++ b/flag.go @@ -3,11 +3,11 @@ package cli import ( "flag" "fmt" - "os" "reflect" "runtime" "strconv" "strings" + "syscall" "time" ) @@ -92,7 +92,7 @@ func (f GenericFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { val.Set(envVal) break } @@ -128,7 +128,7 @@ func (f StringSliceFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { newVal := &StringSlice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) @@ -176,7 +176,7 @@ func (f IntSliceFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { newVal := &IntSlice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) @@ -227,7 +227,7 @@ func (f Int64SliceFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { newVal := &Int64Slice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) @@ -256,7 +256,12 @@ func (f BoolFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { + if envVal == "" { + val = false + break + } + envValBool, err := strconv.ParseBool(envVal) if err == nil { val = envValBool @@ -281,7 +286,12 @@ func (f BoolTFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { + if envVal == "" { + val = false + break + } + envValBool, err := strconv.ParseBool(envVal) if err == nil { val = envValBool @@ -305,7 +315,7 @@ func (f StringFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { f.Value = envVal break } @@ -326,7 +336,7 @@ func (f IntFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseInt(envVal, 0, 64) if err == nil { f.Value = int(envValInt) @@ -350,7 +360,7 @@ func (f Int64Flag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseInt(envVal, 0, 64) if err == nil { f.Value = envValInt @@ -374,7 +384,7 @@ func (f UintFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseUint(envVal, 0, 64) if err == nil { f.Value = uint(envValInt) @@ -398,7 +408,7 @@ func (f Uint64Flag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseUint(envVal, 0, 64) if err == nil { f.Value = uint64(envValInt) @@ -422,7 +432,7 @@ func (f DurationFlag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValDuration, err := time.ParseDuration(envVal) if err == nil { f.Value = envValDuration @@ -446,7 +456,7 @@ func (f Float64Flag) Apply(set *flag.FlagSet) { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) - if envVal := os.Getenv(envVar); envVal != "" { + if envVal, ok := syscall.Getenv(envVar); ok { envValFloat, err := strconv.ParseFloat(envVal, 10) if err == nil { f.Value = float64(envValFloat) diff --git a/flag_test.go b/flag_test.go index a7afcc4..58ada36 100644 --- a/flag_test.go +++ b/flag_test.go @@ -29,6 +29,38 @@ func TestBoolFlagHelpOutput(t *testing.T) { } } +func TestParseBoolFromEnv(t *testing.T) { + var boolFlagTests = []struct { + input string + output bool + }{ + {"", false}, + {"1", true}, + {"false", false}, + {"true", true}, + } + + for _, test := range boolFlagTests { + os.Clearenv() + os.Setenv("DEBUG", test.input) + a := App{ + Flags: []Flag{ + BoolFlag{Name: "debug, d", EnvVar: "DEBUG"}, + }, + Action: func(ctx *Context) error { + if ctx.Bool("debug") != test.output { + t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("debug")) + } + if ctx.Bool("d") != test.output { + t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("d")) + } + return nil + }, + } + a.Run([]string{"run"}) + } +} + var stringFlagTests = []struct { name string usage string @@ -941,6 +973,38 @@ func TestParseMultiBoolFromEnvCascade(t *testing.T) { a.Run([]string{"run"}) } +func TestParseBoolTFromEnv(t *testing.T) { + var boolTFlagTests = []struct { + input string + output bool + }{ + {"", false}, + {"1", true}, + {"false", false}, + {"true", true}, + } + + for _, test := range boolTFlagTests { + os.Clearenv() + os.Setenv("DEBUG", test.input) + a := App{ + Flags: []Flag{ + BoolTFlag{Name: "debug, d", EnvVar: "DEBUG"}, + }, + Action: func(ctx *Context) error { + if ctx.Bool("debug") != test.output { + t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("debug")) + } + if ctx.Bool("d") != test.output { + t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("d")) + } + return nil + }, + } + a.Run([]string{"run"}) + } +} + func TestParseMultiBoolT(t *testing.T) { a := App{ Flags: []Flag{ From e367fafa3d3ce89a09e75d1650a9a73057a9bf36 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 17 Sep 2016 16:54:29 -0700 Subject: [PATCH 05/10] Return an error when parsing environment variables for values fails Currently cli silently (aside from IntSlice and Int64Slice which oddly printed directly to the error stream) ignores failures that occur when parsing environment variables for their value for flags that define environment variables. Instead, we should propogate up the error to the user. This is accomplished in a backwards compatible manner by adding a new, internal, interface which defines an applyWithError function that all flags here define. In v2, when we can modify the interface, we can drop this secondary interface and modify `Apply` to return an error. --- app.go | 12 ++- app_test.go | 24 +++++- command.go | 5 +- context.go | 5 ++ context_test.go | 44 +++++++++- flag.go | 225 ++++++++++++++++++++++++++++++++++++++++-------- flag_test.go | 83 ++++++++++++++---- 7 files changed, 333 insertions(+), 65 deletions(-) diff --git a/app.go b/app.go index a90a4df..95aa5ed 100644 --- a/app.go +++ b/app.go @@ -174,7 +174,11 @@ func (a *App) Run(arguments []string) (err error) { a.Setup() // parse flags - set := flagSet(a.Name, a.Flags) + set, err := flagSet(a.Name, a.Flags) + if err != nil { + return err + } + set.SetOutput(ioutil.Discard) err = set.Parse(arguments[1:]) nerr := normalizeFlags(a.Flags, set) @@ -293,7 +297,11 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } // parse flags - set := flagSet(a.Name, a.Flags) + set, err := flagSet(a.Name, a.Flags) + if err != nil { + return err + } + set.SetOutput(ioutil.Discard) err = set.Parse(ctx.Args().Tail()) nerr := normalizeFlags(a.Flags, set) diff --git a/app_test.go b/app_test.go index 711de47..83d096f 100644 --- a/app_test.go +++ b/app_test.go @@ -1523,7 +1523,11 @@ func TestApp_OnUsageError_WithWrongFlagValue_ForSubcommand(t *testing.T) { func TestHandleAction_WithNonFuncAction(t *testing.T) { app := NewApp() app.Action = 42 - err := HandleAction(app.Action, NewContext(app, flagSet(app.Name, app.Flags), nil)) + fs, err := flagSet(app.Name, app.Flags) + if err != nil { + t.Errorf("error creating FlagSet: %s", err) + } + err = HandleAction(app.Action, NewContext(app, fs, nil)) if err == nil { t.Fatalf("expected to receive error from Run, got none") @@ -1547,7 +1551,11 @@ func TestHandleAction_WithNonFuncAction(t *testing.T) { func TestHandleAction_WithInvalidFuncSignature(t *testing.T) { app := NewApp() app.Action = func() string { return "" } - err := HandleAction(app.Action, NewContext(app, flagSet(app.Name, app.Flags), nil)) + fs, err := flagSet(app.Name, app.Flags) + if err != nil { + t.Errorf("error creating FlagSet: %s", err) + } + err = HandleAction(app.Action, NewContext(app, fs, nil)) if err == nil { t.Fatalf("expected to receive error from Run, got none") @@ -1571,7 +1579,11 @@ func TestHandleAction_WithInvalidFuncSignature(t *testing.T) { func TestHandleAction_WithInvalidFuncReturnSignature(t *testing.T) { app := NewApp() app.Action = func(_ *Context) (int, error) { return 0, nil } - err := HandleAction(app.Action, NewContext(app, flagSet(app.Name, app.Flags), nil)) + fs, err := flagSet(app.Name, app.Flags) + if err != nil { + t.Errorf("error creating FlagSet: %s", err) + } + err = HandleAction(app.Action, NewContext(app, fs, nil)) if err == nil { t.Fatalf("expected to receive error from Run, got none") @@ -1602,5 +1614,9 @@ func TestHandleAction_WithUnknownPanic(t *testing.T) { fn(ctx) return nil } - HandleAction(app.Action, NewContext(app, flagSet(app.Name, app.Flags), nil)) + fs, err := flagSet(app.Name, app.Flags) + if err != nil { + t.Errorf("error creating FlagSet: %s", err) + } + HandleAction(app.Action, NewContext(app, fs, nil)) } diff --git a/command.go b/command.go index 8f1a215..3cc643d 100644 --- a/command.go +++ b/command.go @@ -91,7 +91,10 @@ func (c Command) Run(ctx *Context) (err error) { c.Flags = append(c.Flags, BashCompletionFlag) } - set := flagSet(c.Name, c.Flags) + set, err := flagSet(c.Name, c.Flags) + if err != nil { + return err + } set.SetOutput(ioutil.Discard) if c.SkipFlagParsing { diff --git a/context.go b/context.go index 3893a50..19de0d8 100644 --- a/context.go +++ b/context.go @@ -147,6 +147,11 @@ func (c *Context) Parent() *Context { return c.parentContext } +// value returns the value of the flag coressponding to `name` +func (c *Context) value(name string) interface{} { + return c.flagSet.Lookup(name).Value.(flag.Getter).Get() +} + // Args contains apps console arguments type Args []string diff --git a/context_test.go b/context_test.go index b5bc993..7a6eebf 100644 --- a/context_test.go +++ b/context_test.go @@ -184,7 +184,12 @@ func TestContext_IsSet(t *testing.T) { // XXX Corresponds to hack in context.IsSet for flags with EnvVar field // Should be moved to `flag_test` in v2 func TestContext_IsSet_fromEnv(t *testing.T) { - var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet, passwordIsSet, pIsSet bool + var ( + timeoutIsSet, tIsSet bool + noEnvVarIsSet, nIsSet bool + passwordIsSet, pIsSet bool + unparsableIsSet, uIsSet bool + ) os.Clearenv() os.Setenv("APP_TIMEOUT_SECONDS", "15.5") @@ -192,7 +197,8 @@ func TestContext_IsSet_fromEnv(t *testing.T) { a := App{ Flags: []Flag{ Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"}, - Float64Flag{Name: "password, p", EnvVar: "APP_PASSWORD"}, + StringFlag{Name: "password, p", EnvVar: "APP_PASSWORD"}, + Float64Flag{Name: "unparsable, u", EnvVar: "APP_UNPARSABLE"}, Float64Flag{Name: "no-env-var, n"}, }, Action: func(ctx *Context) error { @@ -200,6 +206,8 @@ func TestContext_IsSet_fromEnv(t *testing.T) { tIsSet = ctx.IsSet("t") passwordIsSet = ctx.IsSet("password") pIsSet = ctx.IsSet("p") + unparsableIsSet = ctx.IsSet("unparsable") + uIsSet = ctx.IsSet("u") noEnvVarIsSet = ctx.IsSet("no-env-var") nIsSet = ctx.IsSet("n") return nil @@ -212,6 +220,11 @@ func TestContext_IsSet_fromEnv(t *testing.T) { expect(t, pIsSet, true) expect(t, noEnvVarIsSet, false) expect(t, nIsSet, false) + + os.Setenv("APP_UNPARSABLE", "foobar") + a.Run([]string{"run"}) + expect(t, unparsableIsSet, false) + expect(t, uIsSet, false) } func TestContext_GlobalIsSet(t *testing.T) { @@ -236,14 +249,22 @@ func TestContext_GlobalIsSet(t *testing.T) { // XXX Corresponds to hack in context.IsSet for flags with EnvVar field // Should be moved to `flag_test` in v2 func TestContext_GlobalIsSet_fromEnv(t *testing.T) { - var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool + var ( + timeoutIsSet, tIsSet bool + noEnvVarIsSet, nIsSet bool + passwordIsSet, pIsSet bool + unparsableIsSet, uIsSet bool + ) os.Clearenv() os.Setenv("APP_TIMEOUT_SECONDS", "15.5") + os.Setenv("APP_PASSWORD", "") a := App{ Flags: []Flag{ Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"}, + StringFlag{Name: "password, p", EnvVar: "APP_PASSWORD"}, Float64Flag{Name: "no-env-var, n"}, + Float64Flag{Name: "unparsable, u", EnvVar: "APP_UNPARSABLE"}, }, Commands: []Command{ { @@ -251,6 +272,10 @@ func TestContext_GlobalIsSet_fromEnv(t *testing.T) { Action: func(ctx *Context) error { timeoutIsSet = ctx.GlobalIsSet("timeout") tIsSet = ctx.GlobalIsSet("t") + passwordIsSet = ctx.GlobalIsSet("password") + pIsSet = ctx.GlobalIsSet("p") + unparsableIsSet = ctx.GlobalIsSet("unparsable") + uIsSet = ctx.GlobalIsSet("u") noEnvVarIsSet = ctx.GlobalIsSet("no-env-var") nIsSet = ctx.GlobalIsSet("n") return nil @@ -258,11 +283,22 @@ func TestContext_GlobalIsSet_fromEnv(t *testing.T) { }, }, } - a.Run([]string{"run", "hello"}) + if err := a.Run([]string{"run", "hello"}); err != nil { + t.Logf("error running Run(): %+v", err) + } expect(t, timeoutIsSet, true) expect(t, tIsSet, true) + expect(t, passwordIsSet, true) + expect(t, pIsSet, true) expect(t, noEnvVarIsSet, false) expect(t, nIsSet, false) + + os.Setenv("APP_UNPARSABLE", "foobar") + if err := a.Run([]string{"run"}); err != nil { + t.Logf("error running Run(): %+v", err) + } + expect(t, unparsableIsSet, false) + expect(t, uIsSet, false) } func TestContext_NumFlags(t *testing.T) { diff --git a/flag.go b/flag.go index 5e565fb..3e63cf0 100644 --- a/flag.go +++ b/flag.go @@ -62,13 +62,29 @@ type Flag interface { GetName() string } -func flagSet(name string, flags []Flag) *flag.FlagSet { +// errorableFlag is an interface that allows us to return errors during apply +// it allows flags defined in this library to return errors in a fashion backwards compatible +// TODO remove in v2 and modify the existing Flag interface to return errors +type errorableFlag interface { + Flag + + applyWithError(*flag.FlagSet) error +} + +func flagSet(name string, flags []Flag) (*flag.FlagSet, error) { set := flag.NewFlagSet(name, flag.ContinueOnError) for _, f := range flags { - f.Apply(set) + //TODO remove in v2 when errorableFlag is removed + if f, ok := f.(errorableFlag); ok { + if err := f.applyWithError(set); err != nil { + return nil, err + } + } else { + f.Apply(set) + } } - return set + return set, nil } func eachName(longName string, fn func(string)) { @@ -87,13 +103,22 @@ type Generic interface { // Apply takes the flagset and calls Set on the generic flag with the value // provided by the user for parsing by the flag +// Ignores parsing errors func (f GenericFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError takes the flagset and calls Set on the generic flag with the value +// provided by the user for parsing by the flag +func (f GenericFlag) applyWithError(set *flag.FlagSet) error { val := f.Value if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { - val.Set(envVal) + if err := val.Set(envVal); err != nil { + return fmt.Errorf("could not parse %s as value for flag %s: %s", envVal, f.Name, err) + } break } } @@ -102,9 +127,11 @@ func (f GenericFlag) Apply(set *flag.FlagSet) { eachName(f.Name, func(name string) { set.Var(f.Value, name, f.Usage) }) + + return nil } -// StringSlice is an opaque type for []string to satisfy flag.Value +// StringSlice is an opaque type for []string to satisfy flag.Value and flag.Getter type StringSlice []string // Set appends the string value to the list of values @@ -123,8 +150,19 @@ func (f *StringSlice) Value() []string { return *f } +// Get returns the slice of strings set by this flag +func (f *StringSlice) Get() interface{} { + return *f +} + // Apply populates the flag given the flag set and environment +// Ignores errors func (f StringSliceFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f StringSliceFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) @@ -132,7 +170,9 @@ func (f StringSliceFlag) Apply(set *flag.FlagSet) { newVal := &StringSlice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) - newVal.Set(s) + if err := newVal.Set(s); err != nil { + return fmt.Errorf("could not parse %s as string value for flag %s: %s", envVal, f.Name, err) + } } f.Value = newVal break @@ -146,9 +186,11 @@ func (f StringSliceFlag) Apply(set *flag.FlagSet) { } set.Var(f.Value, name, f.Usage) }) + + return nil } -// IntSlice is an opaque type for []int to satisfy flag.Value +// IntSlice is an opaque type for []int to satisfy flag.Value and flag.Getter type IntSlice []int // Set parses the value into an integer and appends it to the list of values @@ -171,8 +213,19 @@ func (f *IntSlice) Value() []int { return *f } +// Get returns the slice of ints set by this flag +func (f *IntSlice) Get() interface{} { + return *f +} + // Apply populates the flag given the flag set and environment +// Ignores errors func (f IntSliceFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f IntSliceFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) @@ -180,9 +233,8 @@ func (f IntSliceFlag) Apply(set *flag.FlagSet) { newVal := &IntSlice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) - err := newVal.Set(s) - if err != nil { - fmt.Fprintf(ErrWriter, err.Error()) + if err := newVal.Set(s); err != nil { + return fmt.Errorf("could not parse %s as int slice value for flag %s: %s", envVal, f.Name, err) } } f.Value = newVal @@ -197,9 +249,11 @@ func (f IntSliceFlag) Apply(set *flag.FlagSet) { } set.Var(f.Value, name, f.Usage) }) + + return nil } -// Int64Slice is an opaque type for []int to satisfy flag.Value +// Int64Slice is an opaque type for []int to satisfy flag.Value and flag.Getter type Int64Slice []int64 // Set parses the value into an integer and appends it to the list of values @@ -222,8 +276,19 @@ func (f *Int64Slice) Value() []int64 { return *f } +// Get returns the slice of ints set by this flag +func (f *Int64Slice) Get() interface{} { + return *f +} + // Apply populates the flag given the flag set and environment +// Ignores errors func (f Int64SliceFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f Int64SliceFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) @@ -231,9 +296,8 @@ func (f Int64SliceFlag) Apply(set *flag.FlagSet) { newVal := &Int64Slice{} for _, s := range strings.Split(envVal, ",") { s = strings.TrimSpace(s) - err := newVal.Set(s) - if err != nil { - fmt.Fprintf(ErrWriter, err.Error()) + if err := newVal.Set(s); err != nil { + return fmt.Errorf("could not parse %s as int64 slice value for flag %s: %s", envVal, f.Name, err) } } f.Value = newVal @@ -248,10 +312,17 @@ func (f Int64SliceFlag) Apply(set *flag.FlagSet) { } set.Var(f.Value, name, f.Usage) }) + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f BoolFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f BoolFlag) applyWithError(set *flag.FlagSet) error { val := false if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { @@ -263,9 +334,11 @@ func (f BoolFlag) Apply(set *flag.FlagSet) { } envValBool, err := strconv.ParseBool(envVal) - if err == nil { - val = envValBool + if err != nil { + return fmt.Errorf("could not parse %s as bool value for flag %s: %s", envVal, f.Name, err) } + + val = envValBool break } } @@ -278,10 +351,18 @@ func (f BoolFlag) Apply(set *flag.FlagSet) { } set.Bool(name, val, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f BoolTFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f BoolTFlag) applyWithError(set *flag.FlagSet) error { val := true if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { @@ -293,10 +374,12 @@ func (f BoolTFlag) Apply(set *flag.FlagSet) { } envValBool, err := strconv.ParseBool(envVal) - if err == nil { - val = envValBool - break + if err != nil { + return fmt.Errorf("could not parse %s as bool value for flag %s: %s", envVal, f.Name, err) } + + val = envValBool + break } } } @@ -308,10 +391,18 @@ func (f BoolTFlag) Apply(set *flag.FlagSet) { } set.Bool(name, val, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f StringFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f StringFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) @@ -329,19 +420,28 @@ func (f StringFlag) Apply(set *flag.FlagSet) { } set.String(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f IntFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f IntFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseInt(envVal, 0, 64) - if err == nil { - f.Value = int(envValInt) - break + if err != nil { + return fmt.Errorf("could not parse %s as int value for flag %s: %s", envVal, f.Name, err) } + f.Value = int(envValInt) + break } } } @@ -353,19 +453,29 @@ func (f IntFlag) Apply(set *flag.FlagSet) { } set.Int(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f Int64Flag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f Int64Flag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseInt(envVal, 0, 64) - if err == nil { - f.Value = envValInt - break + if err != nil { + return fmt.Errorf("could not parse %s as int value for flag %s: %s", envVal, f.Name, err) } + + f.Value = envValInt + break } } } @@ -377,19 +487,29 @@ func (f Int64Flag) Apply(set *flag.FlagSet) { } set.Int64(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f UintFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f UintFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseUint(envVal, 0, 64) - if err == nil { - f.Value = uint(envValInt) - break + if err != nil { + return fmt.Errorf("could not parse %s as uint value for flag %s: %s", envVal, f.Name, err) } + + f.Value = uint(envValInt) + break } } } @@ -401,19 +521,29 @@ func (f UintFlag) Apply(set *flag.FlagSet) { } set.Uint(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f Uint64Flag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f Uint64Flag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValInt, err := strconv.ParseUint(envVal, 0, 64) - if err == nil { - f.Value = uint64(envValInt) - break + if err != nil { + return fmt.Errorf("could not parse %s as uint64 value for flag %s: %s", envVal, f.Name, err) } + + f.Value = uint64(envValInt) + break } } } @@ -425,19 +555,29 @@ func (f Uint64Flag) Apply(set *flag.FlagSet) { } set.Uint64(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f DurationFlag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f DurationFlag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValDuration, err := time.ParseDuration(envVal) - if err == nil { - f.Value = envValDuration - break + if err != nil { + return fmt.Errorf("could not parse %s as duration for flag %s: %s", envVal, f.Name, err) } + + f.Value = envValDuration + break } } } @@ -449,18 +589,29 @@ func (f DurationFlag) Apply(set *flag.FlagSet) { } set.Duration(name, f.Value, f.Usage) }) + + return nil } // Apply populates the flag given the flag set and environment +// Ignores errors func (f Float64Flag) Apply(set *flag.FlagSet) { + f.applyWithError(set) +} + +// applyWithError populates the flag given the flag set and environment +func (f Float64Flag) applyWithError(set *flag.FlagSet) error { if f.EnvVar != "" { for _, envVar := range strings.Split(f.EnvVar, ",") { envVar = strings.TrimSpace(envVar) if envVal, ok := syscall.Getenv(envVar); ok { envValFloat, err := strconv.ParseFloat(envVal, 10) - if err == nil { - f.Value = float64(envValFloat) + if err != nil { + return fmt.Errorf("could not parse %s as float64 value for flag %s: %s", envVal, f.Name, err) } + + f.Value = float64(envValFloat) + break } } } @@ -472,6 +623,8 @@ func (f Float64Flag) Apply(set *flag.FlagSet) { } set.Float64(name, f.Value, f.Usage) }) + + return nil } func visibleFlags(fl []Flag) []Flag { diff --git a/flag_test.go b/flag_test.go index 58ada36..0dd8654 100644 --- a/flag_test.go +++ b/flag_test.go @@ -29,35 +29,78 @@ func TestBoolFlagHelpOutput(t *testing.T) { } } -func TestParseBoolFromEnv(t *testing.T) { - var boolFlagTests = []struct { +func TestFlagsFromEnv(t *testing.T) { + var flagTests = []struct { input string - output bool + output interface{} + flag Flag + err error }{ - {"", false}, - {"1", true}, - {"false", false}, - {"true", true}, + {"", false, BoolFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"1", true, BoolFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"false", false, BoolFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"foobar", true, BoolFlag{Name: "debug", EnvVar: "DEBUG"}, fmt.Errorf(`could not parse foobar as bool value for flag debug: strconv.ParseBool: parsing "foobar": invalid syntax`)}, + + {"", false, BoolTFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"1", true, BoolTFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"false", false, BoolTFlag{Name: "debug", EnvVar: "DEBUG"}, nil}, + {"foobar", true, BoolTFlag{Name: "debug", EnvVar: "DEBUG"}, fmt.Errorf(`could not parse foobar as bool value for flag debug: strconv.ParseBool: parsing "foobar": invalid syntax`)}, + + {"1s", 1 * time.Second, DurationFlag{Name: "time", EnvVar: "TIME"}, nil}, + {"foobar", false, DurationFlag{Name: "time", EnvVar: "TIME"}, fmt.Errorf(`could not parse foobar as duration for flag time: time: invalid duration foobar`)}, + + {"1.2", 1.2, Float64Flag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1", 1.0, Float64Flag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"foobar", 0, Float64Flag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as float64 value for flag seconds: strconv.ParseFloat: parsing "foobar": invalid syntax`)}, + + {"1", int64(1), Int64Flag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2", 0, Int64Flag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2 as int value for flag seconds: strconv.ParseInt: parsing "1.2": invalid syntax`)}, + {"foobar", 0, Int64Flag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as int value for flag seconds: strconv.ParseInt: parsing "foobar": invalid syntax`)}, + + {"1", 1, IntFlag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2", 0, IntFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2 as int value for flag seconds: strconv.ParseInt: parsing "1.2": invalid syntax`)}, + {"foobar", 0, IntFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as int value for flag seconds: strconv.ParseInt: parsing "foobar": invalid syntax`)}, + + {"1,2", IntSlice{1, 2}, IntSliceFlag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2,2", IntSlice{}, IntSliceFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2,2 as int slice value for flag seconds: strconv.ParseInt: parsing "1.2": invalid syntax`)}, + {"foobar", IntSlice{}, IntSliceFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as int slice value for flag seconds: strconv.ParseInt: parsing "foobar": invalid syntax`)}, + + {"1,2", Int64Slice{1, 2}, Int64SliceFlag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2,2", Int64Slice{}, Int64SliceFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2,2 as int64 slice value for flag seconds: strconv.ParseInt: parsing "1.2": invalid syntax`)}, + {"foobar", Int64Slice{}, Int64SliceFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as int64 slice value for flag seconds: strconv.ParseInt: parsing "foobar": invalid syntax`)}, + + {"foo", "foo", StringFlag{Name: "name", EnvVar: "NAME"}, nil}, + + {"foo,bar", StringSlice{"foo", "bar"}, StringSliceFlag{Name: "names", EnvVar: "NAMES"}, nil}, + + {"1", uint(1), UintFlag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2", 0, UintFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2 as uint value for flag seconds: strconv.ParseUint: parsing "1.2": invalid syntax`)}, + {"foobar", 0, UintFlag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as uint value for flag seconds: strconv.ParseUint: parsing "foobar": invalid syntax`)}, + + {"1", uint64(1), Uint64Flag{Name: "seconds", EnvVar: "SECONDS"}, nil}, + {"1.2", 0, Uint64Flag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse 1.2 as uint64 value for flag seconds: strconv.ParseUint: parsing "1.2": invalid syntax`)}, + {"foobar", 0, Uint64Flag{Name: "seconds", EnvVar: "SECONDS"}, fmt.Errorf(`could not parse foobar as uint64 value for flag seconds: strconv.ParseUint: parsing "foobar": invalid syntax`)}, + + {"foo,bar", &Parser{"foo", "bar"}, GenericFlag{Name: "names", Value: &Parser{}, EnvVar: "NAMES"}, nil}, } - for _, test := range boolFlagTests { + for _, test := range flagTests { os.Clearenv() - os.Setenv("DEBUG", test.input) + os.Setenv(reflect.ValueOf(test.flag).FieldByName("EnvVar").String(), test.input) a := App{ - Flags: []Flag{ - BoolFlag{Name: "debug, d", EnvVar: "DEBUG"}, - }, + Flags: []Flag{test.flag}, Action: func(ctx *Context) error { - if ctx.Bool("debug") != test.output { - t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("debug")) - } - if ctx.Bool("d") != test.output { - t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.Bool("d")) + if !reflect.DeepEqual(ctx.value(test.flag.GetName()), test.output) { + t.Errorf("expected %+v to be parsed as %+v, instead was %+v", test.input, test.output, ctx.value(test.flag.GetName())) } return nil }, } - a.Run([]string{"run"}) + + err := a.Run([]string{"run"}) + if !reflect.DeepEqual(test.err, err) { + t.Errorf("expected error %s, got error %s", test.err, err) + } } } @@ -1100,6 +1143,10 @@ func (p *Parser) String() string { return fmt.Sprintf("%s,%s", p[0], p[1]) } +func (p *Parser) Get() interface{} { + return p +} + func TestParseGeneric(t *testing.T) { a := App{ Flags: []Flag{ From b4a64dc08db6f198cd94c009885a6a83c0ad14c5 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 12 Nov 2016 20:01:44 -0800 Subject: [PATCH 06/10] Add windows implementation of Clearenv for tests Apparently `Clearenv` in Windows just sets the variables to "" --- context_test.go | 4 ++-- helpers_unix_test.go | 9 +++++++++ helpers_windows_test.go | 20 ++++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 helpers_unix_test.go create mode 100644 helpers_windows_test.go diff --git a/context_test.go b/context_test.go index 7a6eebf..a1ab05b 100644 --- a/context_test.go +++ b/context_test.go @@ -191,7 +191,7 @@ func TestContext_IsSet_fromEnv(t *testing.T) { unparsableIsSet, uIsSet bool ) - os.Clearenv() + clearenv() os.Setenv("APP_TIMEOUT_SECONDS", "15.5") os.Setenv("APP_PASSWORD", "") a := App{ @@ -256,7 +256,7 @@ func TestContext_GlobalIsSet_fromEnv(t *testing.T) { unparsableIsSet, uIsSet bool ) - os.Clearenv() + clearenv() os.Setenv("APP_TIMEOUT_SECONDS", "15.5") os.Setenv("APP_PASSWORD", "") a := App{ diff --git a/helpers_unix_test.go b/helpers_unix_test.go new file mode 100644 index 0000000..ae27fc5 --- /dev/null +++ b/helpers_unix_test.go @@ -0,0 +1,9 @@ +// +build darwin dragonfly freebsd linux netbsd openbsd solaris + +package cli + +import "os" + +func clearenv() { + os.Clearenv() +} diff --git a/helpers_windows_test.go b/helpers_windows_test.go new file mode 100644 index 0000000..4eb84f9 --- /dev/null +++ b/helpers_windows_test.go @@ -0,0 +1,20 @@ +package cli + +import ( + "os" + "syscall" +) + +// os.Clearenv() doesn't actually unset variables on Windows +// See: https://github.com/golang/go/issues/17902 +func clearenv() { + for _, s := range os.Environ() { + for j := 1; j < len(s); j++ { + if s[j] == '=' { + keyp, _ := syscall.UTF16PtrFromString(s[0:j]) + syscall.SetEnvironmentVariable(keyp, nil) + break + } + } + } +} From de551c44504d08ca37fb9b1fcfeebcd9799f1656 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sun, 28 Aug 2016 17:43:50 -0700 Subject: [PATCH 07/10] Backport removal of deprecation warnings #508 --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b0d0ee..5cbbe5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ ### Changed - Raise minimum tested/supported Go version to 1.2+ +## [1.18.1] - 2016-08-28 +### Fixed +- Removed deprecation warnings to STDERR to avoid them leaking to the end-user (backported) + ## [1.18.0] - 2016-06-27 ### Added - `./runtests` test runner with coverage tracking by default @@ -29,6 +33,10 @@ - No longer swallows `panic`s that occur within the `Action`s themselves when detecting the signature of the `Action` field +## [1.17.1] - 2016-08-28 +### Fixed +- Removed deprecation warnings to STDERR to avoid them leaking to the end-user + ## [1.17.0] - 2016-05-09 ### Added - Pluggable flag-level help text rendering via `cli.DefaultFlagStringFunc` @@ -50,6 +58,10 @@ - cleanups based on [Go Report Card feedback](https://goreportcard.com/report/github.com/urfave/cli) +## [1.16.1] - 2016-08-28 +### Fixed +- Removed deprecation warnings to STDERR to avoid them leaking to the end-user + ## [1.16.0] - 2016-05-02 ### Added - `Hidden` field on all flag struct types to omit from generated help text From 4c2360f9755dd0d69902a9f7bdc64aef4dd30d24 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sun, 11 Sep 2016 20:17:58 -0700 Subject: [PATCH 08/10] Prepare CHANGELOG for v1.19.0 --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cbbe5b..c5661f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,14 +3,34 @@ **ATTN**: This project uses [semantic versioning](http://semver.org/). ## [Unreleased] + + +## [1.19.0] - 2016-09-11 ### Added - Flag type code generation via `go generate` - Write to stderr and exit 1 if action returns non-nil error - Added support for TOML to the `altsrc` loader +- `SkipArgReorder` was added to allow users to skip the argument reordering. + This is useful if you want to consider all "flags" after an argument as + arguments rather than flags (the default behavior of the stdlib `flag` + library). This is backported functionality from the [removal of the flag + reordering](https://github.com/urfave/cli/pull/398) in the unreleased version + 2 ### Changed - Raise minimum tested/supported Go version to 1.2+ +### Fixed +- `App.Metadata` is initialized automatically now (previously was `nil` unless initialized) +- Correctly show help message if `-h` is provided to a subcommand +- `context.(Global)IsSet` now respects environment variables. Previously it + would return `false` if a flag was specified in the environment rather than + as an argument +- Removed deprecation warnings to STDERR to avoid them leaking to the end-user +- `altsrc`s import paths were updated to use `gopkg.in/urfave/cli.v1`. This + fixes issues that occurred when `gopkg.in/urfave/cli.v1` was imported as well + as `altsrc` where Go would complain that the types didn't match + ## [1.18.1] - 2016-08-28 ### Fixed - Removed deprecation warnings to STDERR to avoid them leaking to the end-user (backported) From fa7ccb1447a78d77447007b830b81cbb0cec7c89 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 12 Nov 2016 13:53:26 -0800 Subject: [PATCH 09/10] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5661f7..b030967 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ - Raise minimum tested/supported Go version to 1.2+ ### Fixed +- If no action is specified on a command or app, the help is now printed instead of `panic`ing - `App.Metadata` is initialized automatically now (previously was `nil` unless initialized) - Correctly show help message if `-h` is provided to a subcommand - `context.(Global)IsSet` now respects environment variables. Previously it From dcdea39be2d678ff387fa40a6cf63f430dc3d805 Mon Sep 17 00:00:00 2001 From: Jesse Szwedko Date: Sat, 12 Nov 2016 14:01:13 -0800 Subject: [PATCH 10/10] Add additional entries to changelog for v.19.0 release --- CHANGELOG.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b030967..f672d59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,11 @@ ## [Unreleased] -## [1.19.0] - 2016-09-11 +## [1.19.0] - 2016-11-12 ### Added +- `FlagsByName` was added to make it easy to sort flags (e.g. `sort.Sort(cli.FlagsByName(app.Flags))`) +- A `Description` field was added to `App` for a more detailed description of + the application (similar to the existing `Description` field on `Command`) - Flag type code generation via `go generate` - Write to stderr and exit 1 if action returns non-nil error - Added support for TOML to the `altsrc` loader @@ -21,6 +24,12 @@ - Raise minimum tested/supported Go version to 1.2+ ### Fixed +- Consider empty environment variables as set (previously environment variables + with the equivalent of `""` would be skipped rather than their value used). +- Return an error if the value in a given environment variable cannot be parsed + as the flag type. Previously these errors were silently swallowed. +- Print full error when an invalid flag is specified (which includes the invalid flag) +- `App.Writer` defaults to `stdout` when `nil` - If no action is specified on a command or app, the help is now printed instead of `panic`ing - `App.Metadata` is initialized automatically now (previously was `nil` unless initialized) - Correctly show help message if `-h` is provided to a subcommand