From 62597fe929c72e20c3661d06b206a007526effb0 Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Thu, 26 Dec 2019 14:05:50 +0100 Subject: [PATCH 01/12] simplify code - eliminate type assertions "gosimple" linter says: S1034: assigning the result of this type assertion to a variable (switch f := f.(type)) could eliminate the following type assertions: flag.go:267:26 flag.go:270:28 flag.go:273:30 flag.go:276:29 --- flag.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/flag.go b/flag.go index ec128fd..077a6dd 100644 --- a/flag.go +++ b/flag.go @@ -261,19 +261,19 @@ func flagValue(f Flag) reflect.Value { func stringifyFlag(f Flag) string { fv := flagValue(f) - switch f.(type) { + switch f := f.(type) { case *IntSliceFlag: return withEnvHint(flagStringSliceField(f, "EnvVars"), - stringifyIntSliceFlag(f.(*IntSliceFlag))) + stringifyIntSliceFlag(f)) case *Int64SliceFlag: return withEnvHint(flagStringSliceField(f, "EnvVars"), - stringifyInt64SliceFlag(f.(*Int64SliceFlag))) + stringifyInt64SliceFlag(f)) case *Float64SliceFlag: return withEnvHint(flagStringSliceField(f, "EnvVars"), - stringifyFloat64SliceFlag(f.(*Float64SliceFlag))) + stringifyFloat64SliceFlag(f)) case *StringSliceFlag: return withEnvHint(flagStringSliceField(f, "EnvVars"), - stringifyStringSliceFlag(f.(*StringSliceFlag))) + stringifyStringSliceFlag(f)) } placeholder, usage := unquoteUsage(fv.FieldByName("Usage").String()) From 61ac69a2782cfae5f131a1579f82590650a234ef Mon Sep 17 00:00:00 2001 From: "lynn [they]" Date: Fri, 27 Dec 2019 04:31:38 -0800 Subject: [PATCH 02/12] Update v1-bug-report.md --- .github/ISSUE_TEMPLATE/v1-bug-report.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/v1-bug-report.md b/.github/ISSUE_TEMPLATE/v1-bug-report.md index f051613..e524619 100644 --- a/.github/ISSUE_TEMPLATE/v1-bug-report.md +++ b/.github/ISSUE_TEMPLATE/v1-bug-report.md @@ -32,9 +32,13 @@ A clear and concise description of what the bug is. Describe the steps or code required to reproduce the behavior +## Observed behavior + +What did you see happen immediately after the reproduction steps above? + ## Expected behavior -A clear and concise description of what you expected to happen. +What would you have expected to happen immediately after the reproduction steps above? ## Additional context From 13f3b2949716f4a65109e53d1014e5d6d0ecf76a Mon Sep 17 00:00:00 2001 From: "lynn [they]" Date: Fri, 27 Dec 2019 04:32:02 -0800 Subject: [PATCH 03/12] Update v2-bug-report.md --- .github/ISSUE_TEMPLATE/v2-bug-report.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/v2-bug-report.md b/.github/ISSUE_TEMPLATE/v2-bug-report.md index 4d60eee..34f2350 100644 --- a/.github/ISSUE_TEMPLATE/v2-bug-report.md +++ b/.github/ISSUE_TEMPLATE/v2-bug-report.md @@ -32,9 +32,9 @@ A clear and concise description of what the bug is. Describe the steps or code required to reproduce the behavior -## Expected behavior +## Observed behavior -A clear and concise description of what you expected to happen. +What did you see happen immediately after the reproduction steps above? ## Additional context From 726724e85082a86fbe2099a8bb20832db5f2d408 Mon Sep 17 00:00:00 2001 From: "lynn [they]" Date: Fri, 27 Dec 2019 04:52:51 -0800 Subject: [PATCH 04/12] fix bad copy paste, h/t @rliebz --- .github/ISSUE_TEMPLATE/v2-bug-report.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/v2-bug-report.md b/.github/ISSUE_TEMPLATE/v2-bug-report.md index 34f2350..7412f70 100644 --- a/.github/ISSUE_TEMPLATE/v2-bug-report.md +++ b/.github/ISSUE_TEMPLATE/v2-bug-report.md @@ -28,14 +28,14 @@ _**( Put the version of urfave/cli that you are using here )**_ A clear and concise description of what the bug is. -## To reproduce - -Describe the steps or code required to reproduce the behavior - ## Observed behavior What did you see happen immediately after the reproduction steps above? +## Expected behavior + +What would you have expected to happen immediately after the reproduction steps above? + ## Additional context Add any other context about the problem here. From 31ff2100f563f23d020d8740b58f469a92887106 Mon Sep 17 00:00:00 2001 From: "lynn [they]" Date: Fri, 27 Dec 2019 04:54:31 -0800 Subject: [PATCH 05/12] fix more copy paste tragedy --- .github/ISSUE_TEMPLATE/v2-bug-report.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/ISSUE_TEMPLATE/v2-bug-report.md b/.github/ISSUE_TEMPLATE/v2-bug-report.md index 7412f70..8d688a9 100644 --- a/.github/ISSUE_TEMPLATE/v2-bug-report.md +++ b/.github/ISSUE_TEMPLATE/v2-bug-report.md @@ -28,6 +28,10 @@ _**( Put the version of urfave/cli that you are using here )**_ A clear and concise description of what the bug is. +## To reproduce + +Describe the steps or code required to reproduce the behavior + ## Observed behavior What did you see happen immediately after the reproduction steps above? From 8c28ae347ad78af52c36c18d6bb7fe74ce525a1f Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Thu, 26 Dec 2019 17:24:58 +0100 Subject: [PATCH 06/12] refactor flag names - make it more explicitly avoid the using 'reflect' package while getting flag names --- flag.go | 16 +--------------- flag_bool.go | 2 +- flag_duration.go | 2 +- flag_float64.go | 2 +- flag_float64_slice.go | 2 +- flag_generic.go | 2 +- flag_int.go | 2 +- flag_int64.go | 2 +- flag_int64_slice.go | 2 +- flag_int_slice.go | 2 +- flag_path.go | 2 +- flag_string.go | 2 +- flag_string_slice.go | 2 +- flag_timestamp.go | 2 +- flag_uint.go | 2 +- flag_uint64.go | 2 +- 16 files changed, 16 insertions(+), 30 deletions(-) diff --git a/flag.go b/flag.go index 077a6dd..524de42 100644 --- a/flag.go +++ b/flag.go @@ -203,12 +203,9 @@ func withEnvHint(envVars []string, str string) string { return str + envText } -func flagNames(f Flag) []string { +func flagNames(name string, aliases []string) []string { var ret []string - name := flagStringField(f, "Name") - aliases := flagStringSliceField(f, "Aliases") - for _, part := range append([]string{name}, aliases...) { // v1 -> v2 migration warning zone: // Strip off anything after the first found comma or space, which @@ -231,17 +228,6 @@ func flagStringSliceField(f Flag, name string) []string { return []string{} } -func flagStringField(f Flag, name string) string { - fv := flagValue(f) - field := fv.FieldByName(name) - - if field.IsValid() { - return field.String() - } - - return "" -} - func withFileHint(filePath, str string) string { fileText := "" if filePath != "" { diff --git a/flag_bool.go b/flag_bool.go index 6a1da61..bc9ea35 100644 --- a/flag_bool.go +++ b/flag_bool.go @@ -34,7 +34,7 @@ func (f *BoolFlag) String() string { // Names returns the names of the flag func (f *BoolFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_duration.go b/flag_duration.go index 2c34944..22a2e67 100644 --- a/flag_duration.go +++ b/flag_duration.go @@ -34,7 +34,7 @@ func (f *DurationFlag) String() string { // Names returns the names of the flag func (f *DurationFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_float64.go b/flag_float64.go index 31f06f3..91c778c 100644 --- a/flag_float64.go +++ b/flag_float64.go @@ -34,7 +34,7 @@ func (f *Float64Flag) String() string { // Names returns the names of the flag func (f *Float64Flag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_float64_slice.go b/flag_float64_slice.go index 91d2e9d..5f6ae72 100644 --- a/flag_float64_slice.go +++ b/flag_float64_slice.go @@ -90,7 +90,7 @@ func (f *Float64SliceFlag) String() string { // Names returns the names of the flag func (f *Float64SliceFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_generic.go b/flag_generic.go index 2d9baa7..b0c8ff4 100644 --- a/flag_generic.go +++ b/flag_generic.go @@ -39,7 +39,7 @@ func (f *GenericFlag) String() string { // Names returns the names of the flag func (f *GenericFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_int.go b/flag_int.go index be961bf..ac39d4a 100644 --- a/flag_int.go +++ b/flag_int.go @@ -34,7 +34,7 @@ func (f *IntFlag) String() string { // Names returns the names of the flag func (f *IntFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_int64.go b/flag_int64.go index c979119..e099912 100644 --- a/flag_int64.go +++ b/flag_int64.go @@ -34,7 +34,7 @@ func (f *Int64Flag) String() string { // Names returns the names of the flag func (f *Int64Flag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_int64_slice.go b/flag_int64_slice.go index 41aa066..edc3908 100644 --- a/flag_int64_slice.go +++ b/flag_int64_slice.go @@ -91,7 +91,7 @@ func (f *Int64SliceFlag) String() string { // Names returns the names of the flag func (f *Int64SliceFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_int_slice.go b/flag_int_slice.go index 9388978..94479f6 100644 --- a/flag_int_slice.go +++ b/flag_int_slice.go @@ -102,7 +102,7 @@ func (f *IntSliceFlag) String() string { // Names returns the names of the flag func (f *IntSliceFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_path.go b/flag_path.go index a322857..8070dc4 100644 --- a/flag_path.go +++ b/flag_path.go @@ -30,7 +30,7 @@ func (f *PathFlag) String() string { // Names returns the names of the flag func (f *PathFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_string.go b/flag_string.go index bcd8253..400bb53 100644 --- a/flag_string.go +++ b/flag_string.go @@ -31,7 +31,7 @@ func (f *StringFlag) String() string { // Names returns the names of the flag func (f *StringFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_string_slice.go b/flag_string_slice.go index a114a49..71bfef2 100644 --- a/flag_string_slice.go +++ b/flag_string_slice.go @@ -86,7 +86,7 @@ func (f *StringSliceFlag) String() string { // Names returns the names of the flag func (f *StringSliceFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_timestamp.go b/flag_timestamp.go index d24edcd..9fac1d1 100644 --- a/flag_timestamp.go +++ b/flag_timestamp.go @@ -86,7 +86,7 @@ func (f *TimestampFlag) String() string { // Names returns the names of the flag func (f *TimestampFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_uint.go b/flag_uint.go index 9f59238..2e5e76b 100644 --- a/flag_uint.go +++ b/flag_uint.go @@ -34,7 +34,7 @@ func (f *UintFlag) String() string { // Names returns the names of the flag func (f *UintFlag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required diff --git a/flag_uint64.go b/flag_uint64.go index 5bbd1fa..8fc3289 100644 --- a/flag_uint64.go +++ b/flag_uint64.go @@ -34,7 +34,7 @@ func (f *Uint64Flag) String() string { // Names returns the names of the flag func (f *Uint64Flag) Names() []string { - return flagNames(f) + return flagNames(f.Name, f.Aliases) } // IsRequired returns whether or not the flag is required From 94a1912e259ca236fe7772b6843aa57807701170 Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Sun, 29 Dec 2019 16:58:34 +0100 Subject: [PATCH 07/12] reduce stdout logs in tests Keep the stdout clean if all tests are passed. It helps to debug a broken test, because only a failed test prints to output. --- app_test.go | 195 ++++++++++++++++++++++++++---------------------- command_test.go | 5 +- docs_test.go | 2 +- flag_test.go | 2 + 4 files changed, 110 insertions(+), 94 deletions(-) diff --git a/app_test.go b/app_test.go index 53e78b7..2987144 100644 --- a/app_test.go +++ b/app_test.go @@ -612,7 +612,7 @@ func TestApp_UseShortOptionHandling(t *testing.T) { var name string expected := "expectedName" - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true app.Flags = []Flag{ &BoolFlag{Name: "one", Aliases: []string{"o"}}, @@ -633,7 +633,7 @@ func TestApp_UseShortOptionHandling(t *testing.T) { } func TestApp_UseShortOptionHandling_missing_value(t *testing.T) { - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true app.Flags = []Flag{ &StringFlag{Name: "name", Aliases: []string{"n"}}, @@ -648,7 +648,7 @@ func TestApp_UseShortOptionHandlingCommand(t *testing.T) { var name string expected := "expectedName" - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true command := &Command{ Name: "cmd", @@ -673,7 +673,7 @@ func TestApp_UseShortOptionHandlingCommand(t *testing.T) { } func TestApp_UseShortOptionHandlingCommand_missing_value(t *testing.T) { - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true command := &Command{ Name: "cmd", @@ -692,7 +692,7 @@ func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) { var name string expected := "expectedName" - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true command := &Command{ Name: "cmd", @@ -722,7 +722,7 @@ func TestApp_UseShortOptionHandlingSubCommand(t *testing.T) { } func TestApp_UseShortOptionHandlingSubCommand_missing_value(t *testing.T) { - app := NewApp() + app := newTestApp() app.UseShortOptionHandling = true command := &Command{ Name: "cmd", @@ -925,6 +925,7 @@ func TestApp_BeforeFunc(t *testing.T) { Flags: []Flag{ &StringFlag{Name: "opt"}, }, + Writer: ioutil.Discard, } // run with the Before() func succeeding @@ -1186,7 +1187,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { // setup - app := NewApp() + app := newTestApp() app.Flags = test.appFlags app.Commands = test.appCommands @@ -1280,7 +1281,6 @@ func TestApp_OrderOfOperations(t *testing.T) { app := &App{ EnableBashCompletion: true, BashComplete: func(c *Context) { - _, _ = fmt.Fprintf(os.Stderr, "---> BashComplete(%#v)\n", c) counts.Total++ counts.ShellComplete = counts.Total }, @@ -1289,6 +1289,7 @@ func TestApp_OrderOfOperations(t *testing.T) { counts.OnUsageError = counts.Total return errors.New("hay OnUsageError") }, + Writer: ioutil.Discard, } beforeNoError := func(c *Context) error { @@ -1422,49 +1423,50 @@ func TestApp_Run_CommandWithSubcommandHasHelpTopic(t *testing.T) { } for _, flagSet := range subcommandHelpTopics { - t.Logf("==> checking with flags %v", flagSet) + t.Run(fmt.Sprintf("checking with flags %v", flagSet), func(t *testing.T) { - app := &App{} - buf := new(bytes.Buffer) - app.Writer = buf + app := &App{} + buf := new(bytes.Buffer) + app.Writer = buf - subCmdBar := &Command{ - Name: "bar", - Usage: "does bar things", - } - subCmdBaz := &Command{ - Name: "baz", - Usage: "does baz things", - } - cmd := &Command{ - Name: "foo", - Description: "descriptive wall of text about how it does foo things", - Subcommands: []*Command{subCmdBar, subCmdBaz}, - Action: func(c *Context) error { return nil }, - } + subCmdBar := &Command{ + Name: "bar", + Usage: "does bar things", + } + subCmdBaz := &Command{ + Name: "baz", + Usage: "does baz things", + } + cmd := &Command{ + Name: "foo", + Description: "descriptive wall of text about how it does foo things", + Subcommands: []*Command{subCmdBar, subCmdBaz}, + Action: func(c *Context) error { return nil }, + } - app.Commands = []*Command{cmd} - err := app.Run(flagSet) + app.Commands = []*Command{cmd} + err := app.Run(flagSet) - if err != nil { - t.Error(err) - } + if err != nil { + t.Error(err) + } - output := buf.String() + output := buf.String() - if strings.Contains(output, "No help topic for") { - t.Errorf("expect a help topic, got none: \n%q", output) - } + if strings.Contains(output, "No help topic for") { + t.Errorf("expect a help topic, got none: \n%q", output) + } - for _, shouldContain := range []string{ - cmd.Name, cmd.Description, - subCmdBar.Name, subCmdBar.Usage, - subCmdBaz.Name, subCmdBaz.Usage, - } { - if !strings.Contains(output, shouldContain) { - t.Errorf("want help to contain %q, did not: \n%q", shouldContain, output) + for _, shouldContain := range []string{ + cmd.Name, cmd.Description, + subCmdBar.Name, subCmdBar.Usage, + subCmdBaz.Name, subCmdBaz.Usage, + } { + if !strings.Contains(output, shouldContain) { + t.Errorf("want help to contain %q, did not: \n%q", shouldContain, output) + } } - } + }) } } @@ -1610,31 +1612,31 @@ func TestApp_Run_Help(t *testing.T) { var helpArguments = [][]string{{"boom", "--help"}, {"boom", "-h"}, {"boom", "help"}} for _, args := range helpArguments { - buf := new(bytes.Buffer) + t.Run(fmt.Sprintf("checking with arguments %v", args), func(t *testing.T) { - t.Logf("==> checking with arguments %v", args) + buf := new(bytes.Buffer) - app := &App{ - Name: "boom", - Usage: "make an explosive entrance", - Writer: buf, - Action: func(c *Context) error { - buf.WriteString("boom I say!") - return nil - }, - } + app := &App{ + Name: "boom", + Usage: "make an explosive entrance", + Writer: buf, + Action: func(c *Context) error { + buf.WriteString("boom I say!") + return nil + }, + } - err := app.Run(args) - if err != nil { - t.Error(err) - } + err := app.Run(args) + if err != nil { + t.Error(err) + } - output := buf.String() - t.Logf("output: %q\n", buf.Bytes()) + output := buf.String() - if !strings.Contains(output, "boom - make an explosive entrance") { - t.Errorf("want help to contain %q, did not: \n%q", "boom - make an explosive entrance", output) - } + if !strings.Contains(output, "boom - make an explosive entrance") { + t.Errorf("want help to contain %q, did not: \n%q", "boom - make an explosive entrance", output) + } + }) } } @@ -1642,32 +1644,32 @@ func TestApp_Run_Version(t *testing.T) { var versionArguments = [][]string{{"boom", "--version"}, {"boom", "-v"}} for _, args := range versionArguments { - buf := new(bytes.Buffer) + t.Run(fmt.Sprintf("checking with arguments %v", args), func(t *testing.T) { - t.Logf("==> checking with arguments %v", args) + buf := new(bytes.Buffer) - app := &App{ - Name: "boom", - Usage: "make an explosive entrance", - Version: "0.1.0", - Writer: buf, - Action: func(c *Context) error { - buf.WriteString("boom I say!") - return nil - }, - } + app := &App{ + Name: "boom", + Usage: "make an explosive entrance", + Version: "0.1.0", + Writer: buf, + Action: func(c *Context) error { + buf.WriteString("boom I say!") + return nil + }, + } - err := app.Run(args) - if err != nil { - t.Error(err) - } + err := app.Run(args) + if err != nil { + t.Error(err) + } - output := buf.String() - t.Logf("output: %q\n", buf.Bytes()) + output := buf.String() - if !strings.Contains(output, "0.1.0") { - t.Errorf("want version to contain %q, did not: \n%q", "0.1.0", output) - } + if !strings.Contains(output, "0.1.0") { + t.Errorf("want version to contain %q, did not: \n%q", "0.1.0", output) + } + }) } } @@ -1835,6 +1837,7 @@ func TestApp_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) { Action: func(c *Context) error { return nil }, Before: func(c *Context) error { return fmt.Errorf("before error") }, After: func(c *Context) error { return fmt.Errorf("after error") }, + Writer: ioutil.Discard, } err := app.Run([]string{"foo"}) @@ -1979,7 +1982,8 @@ func (c *customBoolFlag) IsSet() bool { func TestCustomFlagsUnused(t *testing.T) { app := &App{ - Flags: []Flag{&customBoolFlag{"custom"}}, + Flags: []Flag{&customBoolFlag{"custom"}}, + Writer: ioutil.Discard, } err := app.Run([]string{"foo"}) @@ -1990,7 +1994,8 @@ func TestCustomFlagsUnused(t *testing.T) { func TestCustomFlagsUsed(t *testing.T) { app := &App{ - Flags: []Flag{&customBoolFlag{"custom"}}, + Flags: []Flag{&customBoolFlag{"custom"}}, + Writer: ioutil.Discard, } err := app.Run([]string{"foo", "--custom=bar"}) @@ -2000,7 +2005,9 @@ func TestCustomFlagsUsed(t *testing.T) { } func TestCustomHelpVersionFlags(t *testing.T) { - app := &App{} + app := &App{ + Writer: ioutil.Discard, + } // Be sure to reset the global flags defer func(helpFlag Flag, versionFlag Flag) { @@ -2018,7 +2025,7 @@ func TestCustomHelpVersionFlags(t *testing.T) { } func TestHandleExitCoder_Default(t *testing.T) { - app := NewApp() + app := newTestApp() fs, err := flagSet(app.Name, app.Flags) if err != nil { t.Errorf("error creating FlagSet: %s", err) @@ -2034,7 +2041,7 @@ func TestHandleExitCoder_Default(t *testing.T) { } func TestHandleExitCoder_Custom(t *testing.T) { - app := NewApp() + app := newTestApp() fs, err := flagSet(app.Name, app.Flags) if err != nil { t.Errorf("error creating FlagSet: %s", err) @@ -2091,6 +2098,7 @@ func TestShellCompletionForIncompleteFlags(t *testing.T) { Action: func(ctx *Context) error { return fmt.Errorf("should not get here") }, + Writer: ioutil.Discard, } err := app.Run([]string{"", "--test-completion", "--" + "generate-bash-completion"}) if err != nil { @@ -2101,7 +2109,7 @@ func TestShellCompletionForIncompleteFlags(t *testing.T) { func TestWhenExitSubCommandWithCodeThenAppQuitUnexpectedly(t *testing.T) { testCode := 104 - app := NewApp() + app := newTestApp() app.Commands = []*Command{ { Name: "cmd", @@ -2120,7 +2128,6 @@ func TestWhenExitSubCommandWithCodeThenAppQuitUnexpectedly(t *testing.T) { var exitCodeFromExitErrHandler int app.ExitErrHandler = func(c *Context, err error) { if exitErr, ok := err.(ExitCoder); ok { - t.Log(exitErr) exitCodeFromExitErrHandler = exitErr.ExitCode() } } @@ -2151,3 +2158,9 @@ func TestWhenExitSubCommandWithCodeThenAppQuitUnexpectedly(t *testing.T) { t.Errorf("exitCodeFromOsExiter valeu should be %v, but its value is %v", testCode, exitCodeFromExitErrHandler) } } + +func newTestApp() *App { + a := NewApp() + a.Writer = ioutil.Discard + return a +} diff --git a/command_test.go b/command_test.go index 8eafe68..765081e 100644 --- a/command_test.go +++ b/command_test.go @@ -93,7 +93,7 @@ func TestParseAndRunShortOpts(t *testing.T) { }, } - app := NewApp() + app := newTestApp() app.Commands = []*Command{cmd} err := app.Run(c.testArgs) @@ -116,6 +116,7 @@ func TestCommand_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) { }, }, }, + Writer: ioutil.Discard, } err := app.Run([]string{"foo", "bar"}) @@ -317,12 +318,12 @@ func TestCommandSkipFlagParsing(t *testing.T) { &StringFlag{Name: "flag"}, }, Action: func(c *Context) error { - fmt.Printf("%+v\n", c.String("flag")) args = c.Args() return nil }, }, }, + Writer: ioutil.Discard, } err := app.Run(c.testArgs) diff --git a/docs_test.go b/docs_test.go index e49dfba..218c331 100644 --- a/docs_test.go +++ b/docs_test.go @@ -6,7 +6,7 @@ import ( ) func testApp() *App { - app := NewApp() + app := newTestApp() app.Name = "greet" app.Flags = []Flag{ &StringFlag{ diff --git a/flag_test.go b/flag_test.go index aaf3a6a..195b593 100644 --- a/flag_test.go +++ b/flag_test.go @@ -1719,6 +1719,7 @@ func TestTimestampFlagApply(t *testing.T) { func TestTimestampFlagApply_Fail_Parse_Wrong_Layout(t *testing.T) { fl := TimestampFlag{Name: "time", Aliases: []string{"t"}, Layout: "randomlayout"} set := flag.NewFlagSet("test", 0) + set.SetOutput(ioutil.Discard) _ = fl.Apply(set) err := set.Parse([]string{"--time", "2006-01-02T15:04:05Z"}) @@ -1728,6 +1729,7 @@ func TestTimestampFlagApply_Fail_Parse_Wrong_Layout(t *testing.T) { func TestTimestampFlagApply_Fail_Parse_Wrong_Time(t *testing.T) { fl := TimestampFlag{Name: "time", Aliases: []string{"t"}, Layout: "Jan 2, 2006 at 3:04pm (MST)"} set := flag.NewFlagSet("test", 0) + set.SetOutput(ioutil.Discard) _ = fl.Apply(set) err := set.Parse([]string{"--time", "2006-01-02T15:04:05Z"}) From ef89298b565a0886dcaeba5173358e97b31afb6e Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Sun, 29 Dec 2019 17:18:04 +0100 Subject: [PATCH 08/12] refactor tests - drop mock writer Use bytes.Buffer instead. --- app_test.go | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/app_test.go b/app_test.go index 53e78b7..5fb109a 100644 --- a/app_test.go +++ b/app_test.go @@ -859,30 +859,12 @@ func TestApp_DefaultStdout(t *testing.T) { } } -type mockWriter struct { - written []byte -} - -func (fw *mockWriter) Write(p []byte) (n int, err error) { - if fw.written == nil { - fw.written = p - } else { - fw.written = append(fw.written, p...) - } - - return len(p), nil -} - -func (fw *mockWriter) GetWritten() (b []byte) { - return fw.written -} - func TestApp_SetStdout(t *testing.T) { - w := &mockWriter{} + var w bytes.Buffer app := &App{ Name: "test", - Writer: w, + Writer: &w, } err := app.Run([]string{"help"}) @@ -891,7 +873,7 @@ func TestApp_SetStdout(t *testing.T) { t.Fatalf("Run error: %s", err) } - if len(w.written) == 0 { + if w.Len() == 0 { t.Error("App did not write output to desired writer.") } } From 0c181376a62ef4c08072be1cda17ae364bd2b3f7 Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Mon, 30 Dec 2019 10:26:21 +0100 Subject: [PATCH 09/12] goimports - fix import order https://github.com/golang/go/wiki/CodeReviewComments#imports --- altsrc/flag_generated.go | 1 + altsrc/flag_test.go | 3 ++- altsrc/input_source_context.go | 3 ++- altsrc/json_command_test.go | 3 ++- altsrc/json_source_context.go | 3 ++- altsrc/map_input_source.go | 3 ++- altsrc/toml_command_test.go | 3 ++- altsrc/yaml_command_test.go | 3 ++- 8 files changed, 15 insertions(+), 7 deletions(-) diff --git a/altsrc/flag_generated.go b/altsrc/flag_generated.go index 29572a6..c9ecea8 100644 --- a/altsrc/flag_generated.go +++ b/altsrc/flag_generated.go @@ -4,6 +4,7 @@ package altsrc import ( "flag" + "github.com/urfave/cli/v2" ) diff --git a/altsrc/flag_test.go b/altsrc/flag_test.go index 80d20ee..34c689e 100644 --- a/altsrc/flag_test.go +++ b/altsrc/flag_test.go @@ -3,12 +3,13 @@ package altsrc import ( "flag" "fmt" - "github.com/urfave/cli/v2" "os" "runtime" "strings" "testing" "time" + + "github.com/urfave/cli/v2" ) type testApplyInputSource struct { diff --git a/altsrc/input_source_context.go b/altsrc/input_source_context.go index 5f60574..a639d8b 100644 --- a/altsrc/input_source_context.go +++ b/altsrc/input_source_context.go @@ -1,8 +1,9 @@ package altsrc import ( - "github.com/urfave/cli/v2" "time" + + "github.com/urfave/cli/v2" ) // InputSourceContext is an interface used to allow diff --git a/altsrc/json_command_test.go b/altsrc/json_command_test.go index bd8e6cf..bd0022b 100644 --- a/altsrc/json_command_test.go +++ b/altsrc/json_command_test.go @@ -2,10 +2,11 @@ package altsrc import ( "flag" - "github.com/urfave/cli/v2" "io/ioutil" "os" "testing" + + "github.com/urfave/cli/v2" ) const ( diff --git a/altsrc/json_source_context.go b/altsrc/json_source_context.go index 31c46cd..d52957f 100644 --- a/altsrc/json_source_context.go +++ b/altsrc/json_source_context.go @@ -3,11 +3,12 @@ package altsrc import ( "encoding/json" "fmt" - "github.com/urfave/cli/v2" "io" "io/ioutil" "strings" "time" + + "github.com/urfave/cli/v2" ) // NewJSONSourceFromFlagFunc returns a func that takes a cli.Context diff --git a/altsrc/map_input_source.go b/altsrc/map_input_source.go index 66bd6e8..661b785 100644 --- a/altsrc/map_input_source.go +++ b/altsrc/map_input_source.go @@ -2,10 +2,11 @@ package altsrc import ( "fmt" - "github.com/urfave/cli/v2" "reflect" "strings" "time" + + "github.com/urfave/cli/v2" ) // MapInputSource implements InputSourceContext to return diff --git a/altsrc/toml_command_test.go b/altsrc/toml_command_test.go index 0ae06cf..b9c7982 100644 --- a/altsrc/toml_command_test.go +++ b/altsrc/toml_command_test.go @@ -2,10 +2,11 @@ package altsrc import ( "flag" - "github.com/urfave/cli/v2" "io/ioutil" "os" "testing" + + "github.com/urfave/cli/v2" ) func TestCommandTomFileTest(t *testing.T) { diff --git a/altsrc/yaml_command_test.go b/altsrc/yaml_command_test.go index 4303e9a..298e4d7 100644 --- a/altsrc/yaml_command_test.go +++ b/altsrc/yaml_command_test.go @@ -2,10 +2,11 @@ package altsrc import ( "flag" - "github.com/urfave/cli/v2" "io/ioutil" "os" "testing" + + "github.com/urfave/cli/v2" ) func TestCommandYamlFileTest(t *testing.T) { From d7e6dde41e45d90f70297ab7050103a6b79cf43b Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Mon, 30 Dec 2019 10:41:15 +0100 Subject: [PATCH 10/12] goimports - fix code format --- flag_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flag_test.go b/flag_test.go index 195b593..f0c444e 100644 --- a/flag_test.go +++ b/flag_test.go @@ -1706,7 +1706,7 @@ func TestTimestamp_set(t *testing.T) { } func TestTimestampFlagApply(t *testing.T) { - expectedResult, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z") + expectedResult, _ := time.Parse(time.RFC3339, "2006-01-02T15:04:05Z") fl := TimestampFlag{Name: "time", Aliases: []string{"t"}, Layout: time.RFC3339} set := flag.NewFlagSet("test", 0) _ = fl.Apply(set) From e17c93181f6b66d8dae96d29330039504c03674f Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Mon, 30 Dec 2019 19:41:39 +0100 Subject: [PATCH 11/12] fix ineffectual assignments Warnings are produced by "ineffassign" linter --- altsrc/json_source_context.go | 30 +++++++++++++++--------------- altsrc/map_input_source_test.go | 2 +- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/altsrc/json_source_context.go b/altsrc/json_source_context.go index d52957f..7d70a2a 100644 --- a/altsrc/json_source_context.go +++ b/altsrc/json_source_context.go @@ -79,9 +79,9 @@ func (x *jsonSource) Duration(name string) (time.Duration, error) { if err != nil { return 0, err } - v, ok := (time.Duration)(0), false - if v, ok = i.(time.Duration); !ok { - return v, fmt.Errorf("unexpected type %T for %q", i, name) + v, ok := i.(time.Duration) + if !ok { + return 0, fmt.Errorf("unexpected type %T for %q", i, name) } return v, nil } @@ -91,9 +91,9 @@ func (x *jsonSource) Float64(name string) (float64, error) { if err != nil { return 0, err } - v, ok := (float64)(0), false - if v, ok = i.(float64); !ok { - return v, fmt.Errorf("unexpected type %T for %q", i, name) + v, ok := i.(float64) + if !ok { + return 0, fmt.Errorf("unexpected type %T for %q", i, name) } return v, nil } @@ -103,9 +103,9 @@ func (x *jsonSource) String(name string) (string, error) { if err != nil { return "", err } - v, ok := "", false - if v, ok = i.(string); !ok { - return v, fmt.Errorf("unexpected type %T for %q", i, name) + v, ok := i.(string) + if !ok { + return "", fmt.Errorf("unexpected type %T for %q", i, name) } return v, nil } @@ -161,9 +161,9 @@ func (x *jsonSource) Generic(name string) (cli.Generic, error) { if err != nil { return nil, err } - v, ok := (cli.Generic)(nil), false - if v, ok = i.(cli.Generic); !ok { - return v, fmt.Errorf("unexpected type %T for %q", i, name) + v, ok := i.(cli.Generic) + if !ok { + return nil, fmt.Errorf("unexpected type %T for %q", i, name) } return v, nil } @@ -173,9 +173,9 @@ func (x *jsonSource) Bool(name string) (bool, error) { if err != nil { return false, err } - v, ok := false, false - if v, ok = i.(bool); !ok { - return v, fmt.Errorf("unexpected type %T for %q", i, name) + v, ok := i.(bool) + if !ok { + return false, fmt.Errorf("unexpected type %T for %q", i, name) } return v, nil } diff --git a/altsrc/map_input_source_test.go b/altsrc/map_input_source_test.go index a921d04..5046d14 100644 --- a/altsrc/map_input_source_test.go +++ b/altsrc/map_input_source_test.go @@ -20,6 +20,6 @@ func TestMapDuration(t *testing.T) { d, err = inputSource.Duration("duration_of_string_type") expect(t, time.Minute, d) expect(t, nil, err) - d, err = inputSource.Duration("duration_of_int_type") + _, err = inputSource.Duration("duration_of_int_type") refute(t, nil, err) } From 5998e27dd71340af37a525ece0fcd28cf79f094b Mon Sep 17 00:00:00 2001 From: Dmitry Kutakov Date: Tue, 31 Dec 2019 16:36:01 +0100 Subject: [PATCH 12/12] remove unused code --- app.go | 11 ----------- context.go | 1 - helpers_test.go | 6 ------ 3 files changed, 18 deletions(-) diff --git a/app.go b/app.go index c04e9af..dd8f1de 100644 --- a/app.go +++ b/app.go @@ -7,7 +7,6 @@ import ( "io" "os" "path/filepath" - "reflect" "sort" "time" ) @@ -485,16 +484,6 @@ func (a *App) VisibleFlags() []Flag { return visibleFlags(a.Flags) } -func (a *App) hasFlag(flag Flag) bool { - for _, f := range a.Flags { - if reflect.DeepEqual(flag, f) { - return true - } - } - - return false -} - func (a *App) errWriter() io.Writer { // When the app ErrWriter is nil use the package level one. if a.ErrWriter == nil { diff --git a/context.go b/context.go index c0c526f..74ed519 100644 --- a/context.go +++ b/context.go @@ -17,7 +17,6 @@ type Context struct { App *App Command *Command shellComplete bool - setFlags map[string]bool flagSet *flag.FlagSet parentContext *Context } diff --git a/helpers_test.go b/helpers_test.go index 767f404..9217e89 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -24,9 +24,3 @@ func expect(t *testing.T, a interface{}, b interface{}) { t.Errorf("(%s:%d) Expected %v (type %v) - Got %v (type %v)", fn, line, b, reflect.TypeOf(b), a, reflect.TypeOf(a)) } } - -func refute(t *testing.T, a interface{}, b interface{}) { - if reflect.DeepEqual(a, b) { - t.Errorf("Did not expect %v (type %v) - Got %v (type %v)", b, reflect.TypeOf(b), a, reflect.TypeOf(a)) - } -}