From 7e0532002650b69f219f34f4614656261be45363 Mon Sep 17 00:00:00 2001 From: Jesse Howarth and Michael Ivey Date: Tue, 2 Dec 2014 17:44:55 +0000 Subject: [PATCH 01/58] Implement required flags --- app.go | 32 ++++++++++++++- command.go | 10 +++++ context.go | 18 +++++++++ flag.go | 114 +++++++++++++++++++++++++++++++++++++---------------- 4 files changed, 138 insertions(+), 36 deletions(-) diff --git a/app.go b/app.go index f4c4af8..31a9070 100644 --- a/app.go +++ b/app.go @@ -87,14 +87,25 @@ func (a *App) Run(arguments []string) error { set.SetOutput(ioutil.Discard) err := set.Parse(arguments[1:]) nerr := normalizeFlags(a.Flags, set) + cerr := checkRequiredFlags(a.Flags, set) + + context := NewContext(a, set, set) + if nerr != nil { fmt.Println(nerr) - context := NewContext(a, set, set) + fmt.Println("") ShowAppHelp(context) fmt.Println("") return nerr } - context := NewContext(a, set, set) + + if cerr != nil { + fmt.Println(cerr) + fmt.Println("") + ShowAppHelp(context) + fmt.Println("") + return cerr + } if err != nil { fmt.Printf("Incorrect Usage.\n\n") @@ -164,10 +175,13 @@ func (a *App) RunAsSubcommand(ctx *Context) error { set.SetOutput(ioutil.Discard) err := set.Parse(ctx.Args().Tail()) nerr := normalizeFlags(a.Flags, set) + cerr := checkRequiredFlags(a.Flags, set) + context := NewContext(a, set, ctx.globalSet) if nerr != nil { fmt.Println(nerr) + fmt.Println("") if len(a.Commands) > 0 { ShowSubcommandHelp(context) } else { @@ -177,6 +191,20 @@ func (a *App) RunAsSubcommand(ctx *Context) error { return nerr } + if cerr != nil { + fmt.Println(cerr) + fmt.Println("") + if len(a.Commands) > 0 { + ShowSubcommandHelp(context) + fmt.Println("subcommands") + } else { + ShowCommandHelp(ctx, context.Args().First()) + fmt.Println("commands") + } + fmt.Println("") + return cerr + } + if err != nil { fmt.Printf("Incorrect Usage.\n\n") ShowSubcommandHelp(context) diff --git a/command.go b/command.go index 5622b38..2ade6fe 100644 --- a/command.go +++ b/command.go @@ -88,6 +88,16 @@ func (c Command) Run(ctx *Context) error { fmt.Println("") return nerr } + + cerr := checkRequiredFlags(c.Flags, set) + if cerr != nil { + fmt.Println(cerr) + fmt.Println("") + ShowCommandHelp(ctx, c.Name) + fmt.Println("") + return cerr + } + context := NewContext(ctx.App, set, ctx.globalSet) if checkCommandCompletions(context, c.Name) { diff --git a/context.go b/context.go index c9f645b..b8fe7a6 100644 --- a/context.go +++ b/context.go @@ -3,6 +3,7 @@ package cli import ( "errors" "flag" + "fmt" "strconv" "strings" "time" @@ -337,3 +338,20 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error { } return nil } + +func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { + visited := make(map[string]bool) + set.Visit(func(f *flag.Flag) { + visited[f.Name] = true + }) + + for _, f := range flags { + if f.IsRequired() { + key := strings.Split(f.getName(), ",")[0] + if !visited[key] { + return fmt.Errorf("Required flag %s not set", f.getName()) + } + } + } + return nil +} diff --git a/flag.go b/flag.go index b30bca3..eb10368 100644 --- a/flag.go +++ b/flag.go @@ -34,6 +34,7 @@ type Flag interface { // Apply Flag settings to the given flag set Apply(*flag.FlagSet) getName() string + IsRequired() bool } func flagSet(name string, flags []Flag) *flag.FlagSet { @@ -61,10 +62,11 @@ type Generic interface { // GenericFlag is the flag type for types implementing Generic type GenericFlag struct { - Name string - Value Generic - Usage string - EnvVar string + Name string + Value Generic + Usage string + EnvVar string + Required bool } func (f GenericFlag) String() string { @@ -88,6 +90,10 @@ func (f GenericFlag) getName() string { return f.Name } +func (f GenericFlag) IsRequired() bool { + return f.Required +} + type StringSlice []string func (f *StringSlice) Set(value string) error { @@ -104,10 +110,11 @@ func (f *StringSlice) Value() []string { } type StringSliceFlag struct { - Name string - Value *StringSlice - Usage string - EnvVar string + Name string + Value *StringSlice + Usage string + EnvVar string + Required bool } func (f StringSliceFlag) String() string { @@ -136,6 +143,10 @@ func (f StringSliceFlag) getName() string { return f.Name } +func (f StringSliceFlag) IsRequired() bool { + return f.Required +} + type IntSlice []int func (f *IntSlice) Set(value string) error { @@ -158,10 +169,11 @@ func (f *IntSlice) Value() []int { } type IntSliceFlag struct { - Name string - Value *IntSlice - Usage string - EnvVar string + Name string + Value *IntSlice + Usage string + EnvVar string + Required bool } func (f IntSliceFlag) String() string { @@ -193,10 +205,15 @@ func (f IntSliceFlag) getName() string { return f.Name } +func (f IntSliceFlag) IsRequired() bool { + return f.Required +} + type BoolFlag struct { - Name string - Usage string - EnvVar string + Name string + Usage string + EnvVar string + Required bool } func (f BoolFlag) String() string { @@ -223,10 +240,15 @@ func (f BoolFlag) getName() string { return f.Name } +func (f BoolFlag) IsRequired() bool { + return f.Required +} + type BoolTFlag struct { - Name string - Usage string - EnvVar string + Name string + Usage string + EnvVar string + Required bool } func (f BoolTFlag) String() string { @@ -253,11 +275,16 @@ func (f BoolTFlag) getName() string { return f.Name } +func (f BoolTFlag) IsRequired() bool { + return f.Required +} + type StringFlag struct { - Name string - Value string - Usage string - EnvVar string + Name string + Value string + Usage string + EnvVar string + Required bool } func (f StringFlag) String() string { @@ -289,11 +316,16 @@ func (f StringFlag) getName() string { return f.Name } +func (f StringFlag) IsRequired() bool { + return f.Required +} + type IntFlag struct { - Name string - Value int - Usage string - EnvVar string + Name string + Value int + Usage string + EnvVar string + Required bool } func (f IntFlag) String() string { @@ -319,11 +351,16 @@ func (f IntFlag) getName() string { return f.Name } +func (f IntFlag) IsRequired() bool { + return f.Required +} + type DurationFlag struct { - Name string - Value time.Duration - Usage string - EnvVar string + Name string + Value time.Duration + Usage string + EnvVar string + Required bool } func (f DurationFlag) String() string { @@ -349,11 +386,16 @@ func (f DurationFlag) getName() string { return f.Name } +func (f DurationFlag) IsRequired() bool { + return f.Required +} + type Float64Flag struct { - Name string - Value float64 - Usage string - EnvVar string + Name string + Value float64 + Usage string + EnvVar string + Required bool } func (f Float64Flag) String() string { @@ -379,6 +421,10 @@ func (f Float64Flag) getName() string { return f.Name } +func (f Float64Flag) IsRequired() bool { + return f.Required +} + func prefixFor(name string) (prefix string) { if len(name) == 1 { prefix = "-" From 73e64a14fde90fc3e85fdebb3647af6024e48de0 Mon Sep 17 00:00:00 2001 From: Jesse Howarth and Michael Ivey Date: Tue, 2 Dec 2014 19:02:56 +0000 Subject: [PATCH 02/58] Add (required) to help of flags that are required. --- flag.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/flag.go b/flag.go index eb10368..83dbba1 100644 --- a/flag.go +++ b/flag.go @@ -70,7 +70,7 @@ type GenericFlag struct { } func (f GenericFlag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s%s %v\t`%v` %s", prefixFor(f.Name), f.Name, f.Value, "-"+f.Name+" option -"+f.Name+" option", f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s%s %v\t`%v` %s", prefixFor(f.Name), f.Name, f.Value, "-"+f.Name+" option -"+f.Name+" option", f.Usage)) } func (f GenericFlag) Apply(set *flag.FlagSet) { @@ -120,7 +120,7 @@ type StringSliceFlag struct { func (f StringSliceFlag) String() string { firstName := strings.Trim(strings.Split(f.Name, ",")[0], " ") pref := prefixFor(firstName) - return withEnvHint(f.EnvVar, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) } func (f StringSliceFlag) Apply(set *flag.FlagSet) { @@ -179,7 +179,7 @@ type IntSliceFlag struct { func (f IntSliceFlag) String() string { firstName := strings.Trim(strings.Split(f.Name, ",")[0], " ") pref := prefixFor(firstName) - return withEnvHint(f.EnvVar, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) } func (f IntSliceFlag) Apply(set *flag.FlagSet) { @@ -217,7 +217,7 @@ type BoolFlag struct { } func (f BoolFlag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) } func (f BoolFlag) Apply(set *flag.FlagSet) { @@ -252,7 +252,7 @@ type BoolTFlag struct { } func (f BoolTFlag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) } func (f BoolTFlag) Apply(set *flag.FlagSet) { @@ -297,7 +297,7 @@ func (f StringFlag) String() string { fmtString = "%s %v\t%v" } - return withEnvHint(f.EnvVar, fmt.Sprintf(fmtString, prefixedNames(f.Name), f.Value, f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf(fmtString, prefixedNames(f.Name), f.Value, f.Usage)) } func (f StringFlag) Apply(set *flag.FlagSet) { @@ -329,7 +329,7 @@ type IntFlag struct { } func (f IntFlag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) } func (f IntFlag) Apply(set *flag.FlagSet) { @@ -364,7 +364,7 @@ type DurationFlag struct { } func (f DurationFlag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) } func (f DurationFlag) Apply(set *flag.FlagSet) { @@ -399,7 +399,7 @@ type Float64Flag struct { } func (f Float64Flag) String() string { - return withEnvHint(f.EnvVar, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) + return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s '%v'\t%v", prefixedNames(f.Name), f.Value, f.Usage)) } func (f Float64Flag) Apply(set *flag.FlagSet) { @@ -454,3 +454,15 @@ func withEnvHint(envVar, str string) string { } return str + envText } + +func withRequiredHint(isRequired bool, str string) string { + if isRequired { + return str + " (required)" + } + + return str +} + +func withHints(envVar string, isRequired bool, str string) string { + return withRequiredHint(isRequired, withEnvHint(envVar, str)) +} From 4b2fcdb1ade79300c56074de8e7a7bf754cd407e Mon Sep 17 00:00:00 2001 From: Jesse Howarth and Michael Ivey Date: Tue, 2 Dec 2014 21:08:24 +0000 Subject: [PATCH 03/58] Add tests for required flags --- required_flags_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 required_flags_test.go diff --git a/required_flags_test.go b/required_flags_test.go new file mode 100644 index 0000000..f9abe7a --- /dev/null +++ b/required_flags_test.go @@ -0,0 +1,60 @@ +package cli + +import ( + "flag" + "testing" +) + +func TestContext_CheckRequiredFlagsSuccess(t *testing.T) { + flags := []Flag{ + StringFlag{ + Name: "required", + Required: true, + }, + StringFlag{ + Name: "optional", + }, + } + + set := flag.NewFlagSet("test", 0) + for _, f := range flags { + f.Apply(set) + } + + e := set.Parse([]string{"--required", "foo"}) + if e != nil { + t.Errorf("Expected no error parsing but there was one: %s", e) + } + + err := checkRequiredFlags(flags, set) + if err != nil { + t.Error("Expected flag parsing to be successful") + } +} + +func TestContext_CheckRequiredFlagsFailure(t *testing.T) { + flags := []Flag{ + StringFlag{ + Name: "required", + Required: true, + }, + StringFlag{ + Name: "optional", + }, + } + + set := flag.NewFlagSet("test", 0) + for _, f := range flags { + f.Apply(set) + } + + e := set.Parse([]string{"--optional", "foo"}) + if e != nil { + t.Errorf("Expected no error parsing but there was one: %s", e) + } + + err := checkRequiredFlags(flags, set) + if err == nil { + t.Error("Expected flag parsing to be unsuccessful") + } +} From cbd95292ac9c4ba7eb30ca121fbe3825ced64f72 Mon Sep 17 00:00:00 2001 From: jhowarth Date: Mon, 2 Mar 2015 11:18:59 -0800 Subject: [PATCH 04/58] Remove debugging --- app.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/app.go b/app.go index 31a9070..e87ed98 100644 --- a/app.go +++ b/app.go @@ -196,10 +196,8 @@ func (a *App) RunAsSubcommand(ctx *Context) error { fmt.Println("") if len(a.Commands) > 0 { ShowSubcommandHelp(context) - fmt.Println("subcommands") } else { ShowCommandHelp(ctx, context.Args().First()) - fmt.Println("commands") } fmt.Println("") return cerr From e67e05f617978eec7bba579a6c86f3d0c11ad96b Mon Sep 17 00:00:00 2001 From: jhowarth Date: Mon, 2 Mar 2015 11:56:29 -0800 Subject: [PATCH 05/58] DRY error handling --- app.go | 19 +++++++++---------- command.go | 20 ++++++++++---------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/app.go b/app.go index e87ed98..d91e6e3 100644 --- a/app.go +++ b/app.go @@ -179,8 +179,9 @@ func (a *App) RunAsSubcommand(ctx *Context) error { context := NewContext(a, set, ctx.globalSet) - if nerr != nil { - fmt.Println(nerr) + // Define here so it closes over the above variables + showErrAndHelp := func(err error) { + fmt.Println(err) fmt.Println("") if len(a.Commands) > 0 { ShowSubcommandHelp(context) @@ -188,18 +189,16 @@ func (a *App) RunAsSubcommand(ctx *Context) error { ShowCommandHelp(ctx, context.Args().First()) } fmt.Println("") + + } + + if nerr != nil { + showErrAndHelp(nerr) return nerr } if cerr != nil { - fmt.Println(cerr) - fmt.Println("") - if len(a.Commands) > 0 { - ShowSubcommandHelp(context) - } else { - ShowCommandHelp(ctx, context.Args().First()) - } - fmt.Println("") + showErrAndHelp(cerr) return cerr } diff --git a/command.go b/command.go index 2ade6fe..833552c 100644 --- a/command.go +++ b/command.go @@ -73,28 +73,28 @@ func (c Command) Run(ctx *Context) error { err = set.Parse(ctx.Args().Tail()) } - if err != nil { - fmt.Printf("Incorrect Usage.\n\n") + // Define here so it closes over the above variables + showErrAndHelp := func(err error) { + fmt.Println(err) + fmt.Println("") ShowCommandHelp(ctx, c.Name) fmt.Println("") + } + + if err != nil { + showErrAndHelp(fmt.Errorf("Incorrect Usage.")) return err } nerr := normalizeFlags(c.Flags, set) if nerr != nil { - fmt.Println(nerr) - fmt.Println("") - ShowCommandHelp(ctx, c.Name) - fmt.Println("") + showErrAndHelp(nerr) return nerr } cerr := checkRequiredFlags(c.Flags, set) if cerr != nil { - fmt.Println(cerr) - fmt.Println("") - ShowCommandHelp(ctx, c.Name) - fmt.Println("") + showErrAndHelp(cerr) return cerr } From 6023f370c1dfea78d4ff99a6ecc6be261347bfc9 Mon Sep 17 00:00:00 2001 From: jhowarth Date: Mon, 2 Mar 2015 12:00:21 -0800 Subject: [PATCH 06/58] dry error messages --- app.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app.go b/app.go index d91e6e3..b2cdb57 100644 --- a/app.go +++ b/app.go @@ -91,26 +91,26 @@ func (a *App) Run(arguments []string) error { context := NewContext(a, set, set) - if nerr != nil { - fmt.Println(nerr) + // Define here so it closes over the above variables + showErrAndHelp := func(err error) { + fmt.Println(err) fmt.Println("") ShowAppHelp(context) fmt.Println("") + } + + if nerr != nil { + showErrAndHelp(nerr) return nerr } if cerr != nil { - fmt.Println(cerr) - fmt.Println("") - ShowAppHelp(context) - fmt.Println("") + showErrAndHelp(cerr) return cerr } if err != nil { - fmt.Printf("Incorrect Usage.\n\n") - ShowAppHelp(context) - fmt.Println("") + showErrAndHelp(fmt.Errorf("Incorrect Usage.")) return err } From 145da3210f41f401b1f42a08385d11ee8a80ec97 Mon Sep 17 00:00:00 2001 From: jhowarth Date: Mon, 2 Mar 2015 12:06:42 -0800 Subject: [PATCH 07/58] don't require flags when the help flag is included --- context.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/context.go b/context.go index b8fe7a6..f9b6f92 100644 --- a/context.go +++ b/context.go @@ -340,6 +340,13 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error { } func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { + // If the help flag is included then none of the other flags are required. + for _, f := range flags { + if f.getName() == "help" { + return nil + } + } + visited := make(map[string]bool) set.Visit(func(f *flag.Flag) { visited[f.Name] = true From aba73cedacbb7b1cec2efb9962460683cd00a90c Mon Sep 17 00:00:00 2001 From: jhowarth Date: Tue, 3 Mar 2015 14:02:42 -0800 Subject: [PATCH 08/58] Copy the writer of the App to the subcommand App --- command.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/command.go b/command.go index b040395..5f2b47c 100644 --- a/command.go +++ b/command.go @@ -166,5 +166,8 @@ func (c Command) startApp(ctx *Context) error { app.Action = helpSubcommand.Action } + // set the writer to the original App's writer + app.Writer = ctx.App.Writer + return app.RunAsSubcommand(ctx) } From ce1630141e70b2ca599a21fd9494e98b88f25b2d Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:18:52 -0700 Subject: [PATCH 09/58] reduce diff??? --- flag.go | 111 +------------------------------------------------------- 1 file changed, 1 insertion(+), 110 deletions(-) diff --git a/flag.go b/flag.go index b4cf6da..de42938 100644 --- a/flag.go +++ b/flag.go @@ -115,22 +115,6 @@ type Generic interface { String() string } -// GenericFlag is the flag type for types implementing Generic -type GenericFlag struct { - Name string - Value Generic - Usage string - EnvVar string - Required bool -} - -// String returns the string representation of the generic flag to display the -// help text to the user (uses the String() method of the generic flag to show -// the value) -func (f GenericFlag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s%s \"%v\"\t%v", prefixFor(f.Name), f.Name, f.Value, f.Usage)) -} - // 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 @@ -178,20 +162,6 @@ func (f *StringSlice) Value() []string { return *f } -type StringSliceFlag struct { - Name string - Value *StringSlice - Usage string - EnvVar string - Required bool -} - -func (f StringSliceFlag) String() string { - firstName := strings.Trim(strings.Split(f.Name, ",")[0], " ") - pref := prefixFor(firstName) - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s [%v]\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) -} - // Get returns the slice of strings set by this flag func (f *StringSlice) Get() interface{} { return *f @@ -257,20 +227,6 @@ func (f *IntSlice) Value() []int { return *f } -type IntSliceFlag struct { - Name string - Value *IntSlice - Usage string - EnvVar string - Required bool -} - -func (f IntSliceFlag) String() string { - firstName := strings.Trim(strings.Split(f.Name, ",")[0], " ") - pref := prefixFor(firstName) - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s [%v]\t%v", prefixedNames(f.Name), pref+firstName+" option "+pref+firstName+" option", f.Usage)) -} - // Get returns the slice of ints set by this flag func (f *IntSlice) Get() interface{} { return *f @@ -331,17 +287,6 @@ func (f IntSliceFlag) IsRequired() bool { return f.Required } -type BoolFlag struct { - Name string - Usage string - EnvVar string - Required bool -} - -func (f BoolFlag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) -} - // Value returns the slice of ints set by this flag func (f *Int64Slice) Value() []int64 { return *f @@ -394,17 +339,6 @@ func (f BoolFlag) IsRequired() bool { return f.Required } -type BoolTFlag struct { - Name string - Usage string - EnvVar string - Required bool -} - -func (f BoolTFlag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s\t%v", prefixedNames(f.Name), f.Usage)) -} - // ApplyWithError populates the flag given the flag set and environment func (f BoolFlag) ApplyWithError(set *flag.FlagSet) error { val := false @@ -468,14 +402,6 @@ func (f BoolTFlag) IsRequired() bool { return f.Required } -type StringFlag struct { - Name string - Value string - Usage string - EnvVar string - Required bool -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f StringFlag) Apply(set *flag.FlagSet) { @@ -536,18 +462,6 @@ func (f StringFlag) IsRequired() bool { return f.Required } -type IntFlag struct { - Name string - Value int - Usage string - EnvVar string - Required bool -} - -func (f IntFlag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s \"%v\"\t%v", prefixedNames(f.Name), f.Value, f.Usage)) -} - // ApplyWithError populates the flag given the flag set and environment func (f Int64Flag) ApplyWithError(set *flag.FlagSet) error { if envVal, ok := flagFromFileEnv(f.FilePath, f.EnvVar); ok { @@ -602,18 +516,6 @@ func (f IntFlag) IsRequired() bool { return f.Required } -type DurationFlag struct { - Name string - Value time.Duration - Usage string - EnvVar string - Required bool -} - -func (f DurationFlag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s \"%v\"\t%v", prefixedNames(f.Name), f.Value, f.Usage)) -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f Uint64Flag) Apply(set *flag.FlagSet) { @@ -667,24 +569,13 @@ func (f DurationFlag) ApplyWithError(set *flag.FlagSet) error { set.Duration(name, f.Value, f.Usage) }) return nil + } func (f DurationFlag) IsRequired() bool { return f.Required } -type Float64Flag struct { - Name string - Value float64 - Usage string - EnvVar string - Required bool -} - -func (f Float64Flag) String() string { - return withHints(f.EnvVar, f.Required, fmt.Sprintf("%s \"%v\"\t%v", prefixedNames(f.Name), f.Value, f.Usage)) -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f Float64Flag) Apply(set *flag.FlagSet) { From fa8187f2ce6a7d8258899b46ccfe081c9c0ea6f7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:19:42 -0700 Subject: [PATCH 10/58] reduce diff --- flag.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flag.go b/flag.go index de42938..6e911dc 100644 --- a/flag.go +++ b/flag.go @@ -568,8 +568,8 @@ func (f DurationFlag) ApplyWithError(set *flag.FlagSet) error { } set.Duration(name, f.Value, f.Usage) }) - return nil + return nil } func (f DurationFlag) IsRequired() bool { From e6842c0b7521b5e608da30a4e8a5ed06e6469cf7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:21:05 -0700 Subject: [PATCH 11/58] merge in test file --- flag_test.go | 57 ++++++++++++++++++++++++++++++++++++++- required_flags_test.go | 60 ------------------------------------------ 2 files changed, 56 insertions(+), 61 deletions(-) delete mode 100644 required_flags_test.go diff --git a/flag_test.go b/flag_test.go index da9fd73..45f3ac9 100644 --- a/flag_test.go +++ b/flag_test.go @@ -1,6 +1,7 @@ package cli import ( + "flag" "fmt" "io" "io/ioutil" @@ -1052,7 +1053,7 @@ func TestParseBoolShortOptionHandle(t *testing.T) { a := App{ Commands: []Command{ { - Name: "foobar", + Name: "foobar", UseShortOptionHandling: true, Action: func(ctx *Context) error { if ctx.Bool("serve") != true { @@ -1352,3 +1353,57 @@ func TestFlagFromFile(t *testing.T) { } } } + +func TestContext_CheckRequiredFlagsSuccess(t *testing.T) { + flags := []Flag{ + StringFlag{ + Name: "required", + Required: true, + }, + StringFlag{ + Name: "optional", + }, + } + + set := flag.NewFlagSet("test", 0) + for _, f := range flags { + f.Apply(set) + } + + e := set.Parse([]string{"--required", "foo"}) + if e != nil { + t.Errorf("Expected no error parsing but there was one: %s", e) + } + + err := checkRequiredFlags(flags, set) + if err != nil { + t.Error("Expected flag parsing to be successful") + } +} + +func TestContext_CheckRequiredFlagsFailure(t *testing.T) { + flags := []Flag{ + StringFlag{ + Name: "required", + Required: true, + }, + StringFlag{ + Name: "optional", + }, + } + + set := flag.NewFlagSet("test", 0) + for _, f := range flags { + f.Apply(set) + } + + e := set.Parse([]string{"--optional", "foo"}) + if e != nil { + t.Errorf("Expected no error parsing but there was one: %s", e) + } + + err := checkRequiredFlags(flags, set) + if err == nil { + t.Error("Expected flag parsing to be unsuccessful") + } +} diff --git a/required_flags_test.go b/required_flags_test.go deleted file mode 100644 index f9abe7a..0000000 --- a/required_flags_test.go +++ /dev/null @@ -1,60 +0,0 @@ -package cli - -import ( - "flag" - "testing" -) - -func TestContext_CheckRequiredFlagsSuccess(t *testing.T) { - flags := []Flag{ - StringFlag{ - Name: "required", - Required: true, - }, - StringFlag{ - Name: "optional", - }, - } - - set := flag.NewFlagSet("test", 0) - for _, f := range flags { - f.Apply(set) - } - - e := set.Parse([]string{"--required", "foo"}) - if e != nil { - t.Errorf("Expected no error parsing but there was one: %s", e) - } - - err := checkRequiredFlags(flags, set) - if err != nil { - t.Error("Expected flag parsing to be successful") - } -} - -func TestContext_CheckRequiredFlagsFailure(t *testing.T) { - flags := []Flag{ - StringFlag{ - Name: "required", - Required: true, - }, - StringFlag{ - Name: "optional", - }, - } - - set := flag.NewFlagSet("test", 0) - for _, f := range flags { - f.Apply(set) - } - - e := set.Parse([]string{"--optional", "foo"}) - if e != nil { - t.Errorf("Expected no error parsing but there was one: %s", e) - } - - err := checkRequiredFlags(flags, set) - if err == nil { - t.Error("Expected flag parsing to be unsuccessful") - } -} From f7d5e2c21e4cca02de26a7f448d69f4dac531af7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:22:16 -0700 Subject: [PATCH 12/58] reduce diff --- flag_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flag_test.go b/flag_test.go index 45f3ac9..33c9eee 100644 --- a/flag_test.go +++ b/flag_test.go @@ -1053,7 +1053,7 @@ func TestParseBoolShortOptionHandle(t *testing.T) { a := App{ Commands: []Command{ { - Name: "foobar", + Name: "foobar", UseShortOptionHandling: true, Action: func(ctx *Context) error { if ctx.Bool("serve") != true { From 30a71dc427bc2634f00d9fe315e5717022e0eb66 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:25:52 -0700 Subject: [PATCH 13/58] update Run command --- app.go | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/app.go b/app.go index 8723539..dcc1ee7 100644 --- a/app.go +++ b/app.go @@ -196,17 +196,8 @@ func (a *App) Run(arguments []string) (err error) { err = set.Parse(arguments[1:]) nerr := normalizeFlags(a.Flags, set) cerr := checkRequiredFlags(a.Flags, set) - context := NewContext(a, set, nil) - // Define here so it closes over the above variables - showErrAndHelp := func(err error) { - fmt.Fprintln(a.Writer, err) - fmt.Fprintln(a.Writer) - ShowAppHelp(context) - fmt.Fprintln(a.Writer) - } - if nerr != nil { fmt.Fprintln(a.Writer, nerr) ShowAppHelp(context) @@ -219,7 +210,7 @@ func (a *App) Run(arguments []string) (err error) { } if cerr != nil { - showErrAndHelp(cerr) + ShowAppHelp(context) return cerr } @@ -345,7 +336,7 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } if cerr != nil { - showErrAndHelp(cerr) + ShowAppHelp(context) return cerr } From 9c299e7e8af265e017adf7abf431a0fe0c89dd95 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:28:29 -0700 Subject: [PATCH 14/58] reduce diff --- app.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app.go b/app.go index dcc1ee7..fee45dc 100644 --- a/app.go +++ b/app.go @@ -332,7 +332,7 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } else { ShowCommandHelp(ctx, context.Args().First()) } - fmt.Fprintln(a.Writer, err) + return nerr } if cerr != nil { From 0608059cc709e86905bfd18886d6649275c9937e Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:32:15 -0700 Subject: [PATCH 15/58] reduce diff --- command.go | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/command.go b/command.go index dcce5bd..5bb2ada 100644 --- a/command.go +++ b/command.go @@ -262,37 +262,11 @@ func reorderArgs(args []string) []string { } } - // Define here so it closes over the above variables - showErrAndHelp := func(err error) { - fmt.Fprintln(ctx.App.Writer, err) - fmt.Fprintln(ctx.App.Writer) - ShowCommandHelp(ctx, c.Name) - fmt.Fprintln(ctx.App.Writer) - } - - if err != nil { - showErrAndHelp(fmt.Errorf("Incorrect Usage.")) - return err - } - - nerr := normalizeFlags(c.Flags, set) - if nerr != nil { - showErrAndHelp(nerr) - return nerr - } - - cerr := checkRequiredFlags(c.Flags, set) - if cerr != nil { - showErrAndHelp(cerr) - return cerr - } - - context := NewContext(ctx.App, set, ctx.globalSet) return append(flags, nonflags...) } func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string { - allCharsFlags := func (s string) bool { + allCharsFlags := func(s string) bool { for i := range s { f := set.Lookup(string(s[i])) if f == nil { @@ -400,9 +374,6 @@ func (c Command) startApp(ctx *Context) error { app.Commands[index].commandNamePath = []string{c.Name, cc.Name} } - // set the writer to the original App's writer - app.Writer = ctx.App.Writer - return app.RunAsSubcommand(ctx) } From 3d2d6975b4fffee753c9422f3440d6b7c114ef40 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:32:42 -0700 Subject: [PATCH 16/58] reduce diff --- command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command.go b/command.go index 5bb2ada..3d44404 100644 --- a/command.go +++ b/command.go @@ -266,7 +266,7 @@ func reorderArgs(args []string) []string { } func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string { - allCharsFlags := func(s string) bool { + allCharsFlags := func (s string) bool { for i := range s { f := set.Lookup(string(s[i])) if f == nil { From af627c73c3ddc2d4ff1e4c0847c3355bc0a47c0d Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:34:17 -0700 Subject: [PATCH 17/58] update func name --- context.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/context.go b/context.go index 6fcf7c3..c9a32ff 100644 --- a/context.go +++ b/context.go @@ -290,7 +290,7 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error { func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { // If the help flag is included then none of the other flags are required. for _, f := range flags { - if f.getName() == "help" { + if f.GetName() == "help" { return nil } } @@ -302,9 +302,9 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { for _, f := range flags { if f.IsRequired() { - key := strings.Split(f.getName(), ",")[0] + key := strings.Split(f.GetName(), ",")[0] if !visited[key] { - return fmt.Errorf("Required flag %s not set", f.getName()) + return fmt.Errorf("Required flag %s not set", f.GetName()) } } } From 310bfeb1942571dfe0ac9f60f45e75df11189e4e Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:44:41 -0700 Subject: [PATCH 18/58] add required attr to generator --- generate-flag-types | 1 + 1 file changed, 1 insertion(+) diff --git a/generate-flag-types b/generate-flag-types index 1358857..6c5bbc8 100755 --- a/generate-flag-types +++ b/generate-flag-types @@ -143,6 +143,7 @@ def _write_cli_flag_types(outfile, types): Usage string EnvVar string FilePath string + Required bool Hidden bool """.format(**typedef)) From 62e99ad1c16714cda6c9f8b980dd9483372771e2 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:46:22 -0700 Subject: [PATCH 19/58] add IsRequired to generator --- generate-flag-types | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/generate-flag-types b/generate-flag-types index 6c5bbc8..e0b5aa1 100755 --- a/generate-flag-types +++ b/generate-flag-types @@ -171,6 +171,11 @@ def _write_cli_flag_types(outfile, types): return f.Name }} + // IsRequired returns the whether or not the flag is required + func (f {name}Flag) IsRequired() bool {{ + return f.Required + }} + // {name} looks up the value of a local {name}Flag, returns // {context_default} if not found func (c *Context) {name}(name string) {context_type} {{ From 8a58b7e039e37b0631d6ced0ab3279c319c4d8c8 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 20:47:47 -0700 Subject: [PATCH 20/58] remove manual isRequired funcs --- flag.go | 36 ------------------------------------ 1 file changed, 36 deletions(-) diff --git a/flag.go b/flag.go index 6e911dc..0252ba2 100644 --- a/flag.go +++ b/flag.go @@ -139,10 +139,6 @@ func (f GenericFlag) ApplyWithError(set *flag.FlagSet) error { return nil } -func (f GenericFlag) IsRequired() bool { - return f.Required -} - // StringSlice is an opaque type for []string to satisfy flag.Value and flag.Getter type StringSlice []string @@ -200,10 +196,6 @@ func (f StringSliceFlag) ApplyWithError(set *flag.FlagSet) error { return nil } -func (f StringSliceFlag) IsRequired() bool { - return f.Required -} - // IntSlice is an opaque type for []int to satisfy flag.Value and flag.Getter type IntSlice []int @@ -283,10 +275,6 @@ func (f *Int64Slice) String() string { return fmt.Sprintf("%#v", *f) } -func (f IntSliceFlag) IsRequired() bool { - return f.Required -} - // Value returns the slice of ints set by this flag func (f *Int64Slice) Value() []int64 { return *f @@ -335,10 +323,6 @@ func (f BoolFlag) Apply(set *flag.FlagSet) { f.ApplyWithError(set) } -func (f BoolFlag) IsRequired() bool { - return f.Required -} - // ApplyWithError populates the flag given the flag set and environment func (f BoolFlag) ApplyWithError(set *flag.FlagSet) error { val := false @@ -398,10 +382,6 @@ func (f BoolTFlag) ApplyWithError(set *flag.FlagSet) error { return nil } -func (f BoolTFlag) IsRequired() bool { - return f.Required -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f StringFlag) Apply(set *flag.FlagSet) { @@ -458,10 +438,6 @@ func (f Int64Flag) Apply(set *flag.FlagSet) { f.ApplyWithError(set) } -func (f StringFlag) IsRequired() bool { - return f.Required -} - // ApplyWithError populates the flag given the flag set and environment func (f Int64Flag) ApplyWithError(set *flag.FlagSet) error { if envVal, ok := flagFromFileEnv(f.FilePath, f.EnvVar); ok { @@ -512,10 +488,6 @@ func (f UintFlag) ApplyWithError(set *flag.FlagSet) error { return nil } -func (f IntFlag) IsRequired() bool { - return f.Required -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f Uint64Flag) Apply(set *flag.FlagSet) { @@ -572,10 +544,6 @@ func (f DurationFlag) ApplyWithError(set *flag.FlagSet) error { return nil } -func (f DurationFlag) IsRequired() bool { - return f.Required -} - // Apply populates the flag given the flag set and environment // Ignores errors func (f Float64Flag) Apply(set *flag.FlagSet) { @@ -615,10 +583,6 @@ func visibleFlags(fl []Flag) []Flag { return visible } -func (f Float64Flag) IsRequired() bool { - return f.Required -} - func prefixFor(name string) (prefix string) { if len(name) == 1 { prefix = "-" From 922d2318916c3b59eafde03c36b12551a71f2d51 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 21:28:09 -0700 Subject: [PATCH 21/58] ./generate-flag-types cli -i flag-types.json -o flag_generated.go --- flag_generated.go | 78 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/flag_generated.go b/flag_generated.go index 001576c..a3e4d6e 100644 --- a/flag_generated.go +++ b/flag_generated.go @@ -14,6 +14,7 @@ type BoolFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Destination *bool } @@ -29,6 +30,11 @@ func (f BoolFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f BoolFlag) IsRequired() bool { + return f.Required +} + // Bool looks up the value of a local BoolFlag, returns // false if not found func (c *Context) Bool(name string) bool { @@ -62,6 +68,7 @@ type BoolTFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Destination *bool } @@ -77,6 +84,11 @@ func (f BoolTFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f BoolTFlag) IsRequired() bool { + return f.Required +} + // BoolT looks up the value of a local BoolTFlag, returns // false if not found func (c *Context) BoolT(name string) bool { @@ -110,6 +122,7 @@ type DurationFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value time.Duration Destination *time.Duration @@ -126,6 +139,11 @@ func (f DurationFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f DurationFlag) IsRequired() bool { + return f.Required +} + // Duration looks up the value of a local DurationFlag, returns // 0 if not found func (c *Context) Duration(name string) time.Duration { @@ -159,6 +177,7 @@ type Float64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value float64 Destination *float64 @@ -175,6 +194,11 @@ func (f Float64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Float64Flag) IsRequired() bool { + return f.Required +} + // Float64 looks up the value of a local Float64Flag, returns // 0 if not found func (c *Context) Float64(name string) float64 { @@ -208,6 +232,7 @@ type GenericFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value Generic } @@ -223,6 +248,11 @@ func (f GenericFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f GenericFlag) IsRequired() bool { + return f.Required +} + // Generic looks up the value of a local GenericFlag, returns // nil if not found func (c *Context) Generic(name string) interface{} { @@ -256,6 +286,7 @@ type Int64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value int64 Destination *int64 @@ -272,6 +303,11 @@ func (f Int64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Int64Flag) IsRequired() bool { + return f.Required +} + // Int64 looks up the value of a local Int64Flag, returns // 0 if not found func (c *Context) Int64(name string) int64 { @@ -305,6 +341,7 @@ type IntFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value int Destination *int @@ -321,6 +358,11 @@ func (f IntFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f IntFlag) IsRequired() bool { + return f.Required +} + // Int looks up the value of a local IntFlag, returns // 0 if not found func (c *Context) Int(name string) int { @@ -354,6 +396,7 @@ type IntSliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *IntSlice } @@ -369,6 +412,11 @@ func (f IntSliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f IntSliceFlag) IsRequired() bool { + return f.Required +} + // IntSlice looks up the value of a local IntSliceFlag, returns // nil if not found func (c *Context) IntSlice(name string) []int { @@ -402,6 +450,7 @@ type Int64SliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *Int64Slice } @@ -417,6 +466,11 @@ func (f Int64SliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Int64SliceFlag) IsRequired() bool { + return f.Required +} + // Int64Slice looks up the value of a local Int64SliceFlag, returns // nil if not found func (c *Context) Int64Slice(name string) []int64 { @@ -450,6 +504,7 @@ type StringFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value string Destination *string @@ -466,6 +521,11 @@ func (f StringFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f StringFlag) IsRequired() bool { + return f.Required +} + // String looks up the value of a local StringFlag, returns // "" if not found func (c *Context) String(name string) string { @@ -499,6 +559,7 @@ type StringSliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *StringSlice } @@ -514,6 +575,11 @@ func (f StringSliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f StringSliceFlag) IsRequired() bool { + return f.Required +} + // StringSlice looks up the value of a local StringSliceFlag, returns // nil if not found func (c *Context) StringSlice(name string) []string { @@ -547,6 +613,7 @@ type Uint64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value uint64 Destination *uint64 @@ -563,6 +630,11 @@ func (f Uint64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Uint64Flag) IsRequired() bool { + return f.Required +} + // Uint64 looks up the value of a local Uint64Flag, returns // 0 if not found func (c *Context) Uint64(name string) uint64 { @@ -596,6 +668,7 @@ type UintFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value uint Destination *uint @@ -612,6 +685,11 @@ func (f UintFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f UintFlag) IsRequired() bool { + return f.Required +} + // Uint looks up the value of a local UintFlag, returns // 0 if not found func (c *Context) Uint(name string) uint { From 6a2ae783730e54eb7ea91cf6839ed46446134017 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 11 Jul 2019 21:53:10 -0700 Subject: [PATCH 22/58] backwards compatible RequiredFlag implementation --- context.go | 10 ++++++---- flag.go | 9 ++++++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/context.go b/context.go index c9a32ff..8746d68 100644 --- a/context.go +++ b/context.go @@ -301,10 +301,12 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { }) for _, f := range flags { - if f.IsRequired() { - key := strings.Split(f.GetName(), ",")[0] - if !visited[key] { - return fmt.Errorf("Required flag %s not set", f.GetName()) + if rf, ok := f.(RequiredFlag); ok { + if rf.IsRequired() { + key := strings.Split(f.GetName(), ",")[0] + if !visited[key] { + return fmt.Errorf("Required flag %s not set", f.GetName()) + } } } } diff --git a/flag.go b/flag.go index 0252ba2..71fd729 100644 --- a/flag.go +++ b/flag.go @@ -72,10 +72,17 @@ type Flag interface { fmt.Stringer // Apply Flag settings to the given flag set Apply(*flag.FlagSet) - IsRequired() bool GetName() string } +// RequiredFlag is an interface that allows us to return mark flags as required +// it allows flags defined in this library to be marked as required in a backwards compatible fashion +type RequiredFlag interface { + Flag + + IsRequired() bool +} + // 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 From f6777bf4bf44867abbcaa63a97a67db60469ea80 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 01:03:46 -0700 Subject: [PATCH 23/58] quote the flag name --- context.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/context.go b/context.go index 8746d68..bd6ac6c 100644 --- a/context.go +++ b/context.go @@ -305,7 +305,7 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { if rf.IsRequired() { key := strings.Split(f.GetName(), ",")[0] if !visited[key] { - return fmt.Errorf("Required flag %s not set", f.GetName()) + return fmt.Errorf("Required flag %q not set", f.GetName()) } } } From 550ed20ea429e19b7b132984a6e34c057acabc42 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 01:26:47 -0700 Subject: [PATCH 24/58] update tests --- context_test.go | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ flag_test.go | 55 ------------------------- 2 files changed, 104 insertions(+), 55 deletions(-) diff --git a/context_test.go b/context_test.go index 7acca10..fac1d4b 100644 --- a/context_test.go +++ b/context_test.go @@ -401,3 +401,107 @@ func TestContext_GlobalSet(t *testing.T) { expect(t, c.GlobalInt("int"), 1) expect(t, c.GlobalIsSet("int"), true) } + +func TestCheckRequiredFlags(t *testing.T) { + tdata := []struct { + testCase string + parseInput []string + flags []Flag + expectedAnError bool + }{ + { + testCase: "empty", + }, + { + testCase: "optional", + flags: []Flag{ + StringFlag{Name: "optionalFlag"}, + }, + }, + { + testCase: "required", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + }, + expectedAnError: true, + }, + { + testCase: "required_and_present", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + }, + parseInput: []string{"--requiredFlag", "myinput"}, + }, + { + testCase: "required_and_optional", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "optionalFlag"}, + }, + expectedAnError: true, + }, + { + testCase: "required_and_optional_and_optional_present", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "optionalFlag"}, + }, + parseInput: []string{"--optionalFlag", "myinput"}, + expectedAnError: true, + }, + { + testCase: "required_and_optional_and_required_present", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "optionalFlag"}, + }, + parseInput: []string{"--requiredFlag", "myinput"}, + }, + { + testCase: "two_required", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "requiredFlagTwo", Required: true}, + }, + expectedAnError: true, + }, + { + testCase: "two_required_and_one_present", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "requiredFlagTwo", Required: true}, + }, + parseInput: []string{"--requiredFlag", "myinput"}, + expectedAnError: true, + }, + { + testCase: "two_required_and_both_present", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "requiredFlagTwo", Required: true}, + }, + parseInput: []string{"--requiredFlag", "myinput", "--requiredFlagTwo", "myinput"}, + }, + } + for _, test := range tdata { + t.Run(test.testCase, func(t *testing.T) { + // setup + set := flag.NewFlagSet("test", 0) + for _, flags := range test.flags { + flags.Apply(set) + } + set.Parse(test.parseInput) + + // logic under test + err := checkRequiredFlags(test.flags, set) + + // assertions + if test.expectedAnError && err == nil { + t.Errorf("expected an error, but there was none") + } + if !test.expectedAnError && err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) + } +} diff --git a/flag_test.go b/flag_test.go index 33c9eee..da9fd73 100644 --- a/flag_test.go +++ b/flag_test.go @@ -1,7 +1,6 @@ package cli import ( - "flag" "fmt" "io" "io/ioutil" @@ -1353,57 +1352,3 @@ func TestFlagFromFile(t *testing.T) { } } } - -func TestContext_CheckRequiredFlagsSuccess(t *testing.T) { - flags := []Flag{ - StringFlag{ - Name: "required", - Required: true, - }, - StringFlag{ - Name: "optional", - }, - } - - set := flag.NewFlagSet("test", 0) - for _, f := range flags { - f.Apply(set) - } - - e := set.Parse([]string{"--required", "foo"}) - if e != nil { - t.Errorf("Expected no error parsing but there was one: %s", e) - } - - err := checkRequiredFlags(flags, set) - if err != nil { - t.Error("Expected flag parsing to be successful") - } -} - -func TestContext_CheckRequiredFlagsFailure(t *testing.T) { - flags := []Flag{ - StringFlag{ - Name: "required", - Required: true, - }, - StringFlag{ - Name: "optional", - }, - } - - set := flag.NewFlagSet("test", 0) - for _, f := range flags { - f.Apply(set) - } - - e := set.Parse([]string{"--optional", "foo"}) - if e != nil { - t.Errorf("Expected no error parsing but there was one: %s", e) - } - - err := checkRequiredFlags(flags, set) - if err == nil { - t.Error("Expected flag parsing to be unsuccessful") - } -} From 746866c10daf9425d41140f78ffc518ee4d9ae01 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 03:44:39 -0700 Subject: [PATCH 25/58] add update integration with the help output --- app.go | 11 ++++++----- app_test.go | 44 ++++++++++++++++++++++++++++++++------------ context.go | 7 ------- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/app.go b/app.go index fee45dc..177ed3e 100644 --- a/app.go +++ b/app.go @@ -209,11 +209,6 @@ func (a *App) Run(arguments []string) (err error) { return nil } - if cerr != nil { - ShowAppHelp(context) - return cerr - } - if err != nil { if a.OnUsageError != nil { err := a.OnUsageError(context, err, false) @@ -235,6 +230,12 @@ func (a *App) Run(arguments []string) (err error) { return nil } + if cerr != nil { + fmt.Fprintln(a.Writer, cerr) + ShowAppHelp(context) + return cerr + } + if a.After != nil { defer func() { if afterErr := a.After(context); afterErr != nil { diff --git a/app_test.go b/app_test.go index 629681e..04b515e 100644 --- a/app_test.go +++ b/app_test.go @@ -878,21 +878,41 @@ func TestAppNoHelpFlag(t *testing.T) { } func TestAppHelpPrinter(t *testing.T) { - oldPrinter := HelpPrinter - defer func() { - HelpPrinter = oldPrinter - }() - - var wasCalled = false - HelpPrinter = func(w io.Writer, template string, data interface{}) { - wasCalled = true + tdata := []struct { + testCase string + flags []Flag + }{ + { + testCase: "prints_help_and_does_not_error", + }, + { + testCase: "prints_help_and_does_not_error_when_required_flag_is_present", + flags: []Flag{StringFlag{Name: "flag", Required: true}}, + }, } + for _, test := range tdata { + t.Run(test.testCase, func(t *testing.T) { + oldPrinter := HelpPrinter + defer func() { + HelpPrinter = oldPrinter + }() - app := NewApp() - app.Run([]string{"-h"}) + var wasCalled = false + HelpPrinter = func(w io.Writer, template string, data interface{}) { + wasCalled = true + } - if wasCalled == false { - t.Errorf("Help printer expected to be called, but was not") + app := NewApp() + app.Flags = test.flags + err := app.Run([]string{"testCommand", "-h"}) + + if wasCalled == false { + t.Errorf("Help printer expected to be called, but was not") + } + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) } } diff --git a/context.go b/context.go index bd6ac6c..600307a 100644 --- a/context.go +++ b/context.go @@ -288,13 +288,6 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error { } func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { - // If the help flag is included then none of the other flags are required. - for _, f := range flags { - if f.GetName() == "help" { - return nil - } - } - visited := make(map[string]bool) set.Visit(func(f *flag.Flag) { visited[f.Name] = true From 80d7e91191cfb38c7e3fccbcf2b1320807d4b05d Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 03:51:26 -0700 Subject: [PATCH 26/58] fill out test cases --- app_test.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/app_test.go b/app_test.go index 04b515e..360977c 100644 --- a/app_test.go +++ b/app_test.go @@ -879,15 +879,26 @@ func TestAppNoHelpFlag(t *testing.T) { func TestAppHelpPrinter(t *testing.T) { tdata := []struct { - testCase string - flags []Flag + testCase string + flags []Flag + appRunInput []string }{ { - testCase: "prints_help_and_does_not_error", + testCase: "prints_help_case_one", + appRunInput: []string{""}, }, { - testCase: "prints_help_and_does_not_error_when_required_flag_is_present", - flags: []Flag{StringFlag{Name: "flag", Required: true}}, + testCase: "prints_help_case_two", + appRunInput: []string{"-h"}, + }, + { + testCase: "prints_help_case_three", + appRunInput: []string{"testCommand", "-h"}, + }, + { + testCase: "prints_help_when_required_flag_is_present", + flags: []Flag{StringFlag{Name: "flag", Required: true}}, + appRunInput: []string{"testCommand", "-h"}, }, } for _, test := range tdata { @@ -904,7 +915,7 @@ func TestAppHelpPrinter(t *testing.T) { app := NewApp() app.Flags = test.flags - err := app.Run([]string{"testCommand", "-h"}) + err := app.Run(test.appRunInput) if wasCalled == false { t.Errorf("Help printer expected to be called, but was not") From cf824804c2353572e0f99b098829f2d7ffe2a0ec Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 13:57:06 -0700 Subject: [PATCH 27/58] update tests --- app.go | 9 ++++- app_test.go | 111 ++++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 98 insertions(+), 22 deletions(-) diff --git a/app.go b/app.go index 177ed3e..52c2d85 100644 --- a/app.go +++ b/app.go @@ -10,6 +10,9 @@ import ( "time" ) +// printerFunc is the function signature for fmt.Fprintln +type printerFunc func(io.Writer, ...interface{}) (int, error) + var ( changeLogURL = "https://github.com/urfave/cli/blob/master/CHANGELOG.md" appActionDeprecationURL = fmt.Sprintf("%s#deprecated-cli-app-action-signature", changeLogURL) @@ -20,6 +23,8 @@ var ( errInvalidActionType = NewExitError("ERROR invalid Action type. "+ fmt.Sprintf("Must be `func(*Context`)` or `func(*Context) error). %s", contactSysadmin)+ fmt.Sprintf("See %s", appActionDeprecationURL), 2) + + showFlagError printerFunc = fmt.Fprintln ) // App is the main structure of a cli application. It is recommended that @@ -195,7 +200,6 @@ func (a *App) Run(arguments []string) (err error) { set.SetOutput(ioutil.Discard) err = set.Parse(arguments[1:]) nerr := normalizeFlags(a.Flags, set) - cerr := checkRequiredFlags(a.Flags, set) context := NewContext(a, set, nil) if nerr != nil { @@ -230,8 +234,9 @@ func (a *App) Run(arguments []string) (err error) { return nil } + cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - fmt.Fprintln(a.Writer, cerr) + showFlagError(a.Writer, cerr) ShowAppHelp(context) return cerr } diff --git a/app_test.go b/app_test.go index 360977c..71d9fcc 100644 --- a/app_test.go +++ b/app_test.go @@ -877,56 +877,127 @@ func TestAppNoHelpFlag(t *testing.T) { } } -func TestAppHelpPrinter(t *testing.T) { +func TestRequiredFlagAppRunBehavior(t *testing.T) { tdata := []struct { - testCase string - flags []Flag - appRunInput []string + testCase string + flags []Flag + appRunInput []string + expectedHelpPrinter bool + expectedAnError bool + expectedFlagErrorPrinter bool }{ { - testCase: "prints_help_case_one", - appRunInput: []string{""}, + // expectations: + // - empty input shows the help message + // - empty input explicitly does not error + testCase: "empty_input", + appRunInput: []string{""}, + expectedHelpPrinter: true, + expectedAnError: false, // explicit false for readability (this is the default value) }, { - testCase: "prints_help_case_two", - appRunInput: []string{"-h"}, + // expectations: + // - empty input, when a required flag is present, shows the help message + // - empty input, when a required flag is present, errors + // - empty input, when a required flag is present, show the flag error message + testCase: "empty_input_with_required_flag", + appRunInput: []string{""}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + expectedHelpPrinter: true, + expectedAnError: true, + expectedFlagErrorPrinter: true, }, { - testCase: "prints_help_case_three", - appRunInput: []string{"testCommand", "-h"}, + // expectations: + // - --help input, when a required flag is present, shows the help message + // - --help input, when a required flag is present, explicitly does not error + // - --help input, when a required flag is present, explicitly does not show the flag error message + testCase: "help_input_with_required_flag", + appRunInput: []string{"command", "--help"}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + expectedHelpPrinter: true, + expectedAnError: false, // explicit false for readability (this is the default value) + expectedFlagErrorPrinter: false, // explicit false for readability (this is the default value) }, { - testCase: "prints_help_when_required_flag_is_present", - flags: []Flag{StringFlag{Name: "flag", Required: true}}, - appRunInput: []string{"testCommand", "-h"}, + // expectations: + // - optional input, when a required flag is present, shows the help message + // - optional input, when a required flag is present, errors + // - optional input, when a required flag is present, shows the flag error message + testCase: "optional_input_with_required_flag", + appRunInput: []string{"command", "--optional", "cats"}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, + expectedHelpPrinter: true, + expectedAnError: true, + expectedFlagErrorPrinter: true, }, } for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { + // setup - undo showFlagError mock when finished + oldShowError := showFlagError + defer func() { + showFlagError = oldShowError + }() + // setup - mock showFlagError + var showErrorWasCalled = false + showFlagError = func(writer io.Writer, err ...interface{}) (int, error) { + showErrorWasCalled = true + return 0, nil + } + // setup - undo HelpPrinter mock when finished oldPrinter := HelpPrinter defer func() { HelpPrinter = oldPrinter }() - - var wasCalled = false + // setup - mock HelpPrinter + var helpPrinterWasCalled = false HelpPrinter = func(w io.Writer, template string, data interface{}) { - wasCalled = true + helpPrinterWasCalled = true } - + // setup - app app := NewApp() app.Flags = test.flags + + // logic under test err := app.Run(test.appRunInput) - if wasCalled == false { - t.Errorf("Help printer expected to be called, but was not") + // assertions + if test.expectedHelpPrinter && helpPrinterWasCalled == false { + t.Errorf("HelpPrinter expected to be called, but was not") + } + if test.expectedFlagErrorPrinter && showErrorWasCalled == false { + t.Errorf("showFlagError expected to be called, but was not") + } + if test.expectedAnError && err == nil { + t.Errorf("expected an error, but there was none") } - if err != nil { + if !test.expectedAnError && err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } }) } } +func TestAppHelpPrinter(t *testing.T) { + oldPrinter := HelpPrinter + defer func() { + HelpPrinter = oldPrinter + }() + + var wasCalled = false + HelpPrinter = func(w io.Writer, template string, data interface{}) { + wasCalled = true + } + + app := NewApp() + app.Run([]string{"-h"}) + + if wasCalled == false { + t.Errorf("Help printer expected to be called, but was not") + } +} + func TestApp_VersionPrinter(t *testing.T) { oldPrinter := VersionPrinter defer func() { From 17108e1db49db34480170f575131e642b22bda2d Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 13:59:29 -0700 Subject: [PATCH 28/58] tabs --- app_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app_test.go b/app_test.go index 71d9fcc..6b44654 100644 --- a/app_test.go +++ b/app_test.go @@ -888,7 +888,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }{ { // expectations: - // - empty input shows the help message + // - empty input shows the help message // - empty input explicitly does not error testCase: "empty_input", appRunInput: []string{""}, @@ -897,7 +897,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, { // expectations: - // - empty input, when a required flag is present, shows the help message + // - empty input, when a required flag is present, shows the help message // - empty input, when a required flag is present, errors // - empty input, when a required flag is present, show the flag error message testCase: "empty_input_with_required_flag", @@ -909,7 +909,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, { // expectations: - // - --help input, when a required flag is present, shows the help message + // - --help input, when a required flag is present, shows the help message // - --help input, when a required flag is present, explicitly does not error // - --help input, when a required flag is present, explicitly does not show the flag error message testCase: "help_input_with_required_flag", @@ -921,7 +921,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, { // expectations: - // - optional input, when a required flag is present, shows the help message + // - optional input, when a required flag is present, shows the help message // - optional input, when a required flag is present, errors // - optional input, when a required flag is present, shows the flag error message testCase: "optional_input_with_required_flag", From f00f35ce8c1a6ebd7a3900901dd8b05049fbefc7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sat, 13 Jul 2019 14:02:45 -0700 Subject: [PATCH 29/58] docs --- app_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app_test.go b/app_test.go index 6b44654..e838d30 100644 --- a/app_test.go +++ b/app_test.go @@ -877,6 +877,10 @@ func TestAppNoHelpFlag(t *testing.T) { } } +// TestRequiredFlagAppRunBehavior tests the app-wide behavior of required flags +// and how they interact with the error response and help messages. +// A different test (`TestCheckRequiredFlags`) exists for the more fine grain +// behavior of required flags. func TestRequiredFlagAppRunBehavior(t *testing.T) { tdata := []struct { testCase string From 9293f5b3cc6f5a96a1976bf2f810c957044c5ee8 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 14 Jul 2019 21:00:16 -0700 Subject: [PATCH 30/58] visually shorten logic --- context.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/context.go b/context.go index 600307a..8caf90d 100644 --- a/context.go +++ b/context.go @@ -294,12 +294,10 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { }) for _, f := range flags { - if rf, ok := f.(RequiredFlag); ok { - if rf.IsRequired() { - key := strings.Split(f.GetName(), ",")[0] - if !visited[key] { - return fmt.Errorf("Required flag %q not set", f.GetName()) - } + if rf, ok := f.(RequiredFlag); ok && rf.IsRequired() { + key := strings.Split(f.GetName(), ",")[0] + if !visited[key] { + return fmt.Errorf("Required flag %q not set", f.GetName()) } } } From cdc7af744e07ac8dbb34793f8b392af46ba443f7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 17 Jul 2019 00:16:40 -0700 Subject: [PATCH 31/58] add handling for multiple required flags --- context.go | 21 ++++++++++++++++++++- context_test.go | 23 ++++++++++++++++------- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/context.go b/context.go index 8caf90d..383749d 100644 --- a/context.go +++ b/context.go @@ -293,13 +293,32 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { visited[f.Name] = true }) + var missingFlags []string for _, f := range flags { if rf, ok := f.(RequiredFlag); ok && rf.IsRequired() { key := strings.Split(f.GetName(), ",")[0] if !visited[key] { - return fmt.Errorf("Required flag %q not set", f.GetName()) + missingFlags = append(missingFlags, f.GetName()) } } } + + numberOfMissingFlags := len(missingFlags) + if numberOfMissingFlags == 1 { + return fmt.Errorf("Required flag %q not set", missingFlags[0]) + } + if numberOfMissingFlags >= 2 { + var missingFlagsOutput string + for idx, f := range missingFlags { + // if not the last item, append with a ", " + if idx != numberOfMissingFlags-1 { + missingFlagsOutput = fmt.Sprintf("%s%s, ", missingFlagsOutput, f) + } else { + missingFlagsOutput = fmt.Sprintf("%s%s", missingFlagsOutput, f) + } + } + return fmt.Errorf("Required flags %q not set", missingFlagsOutput) + } + return nil } diff --git a/context_test.go b/context_test.go index fac1d4b..585ca82 100644 --- a/context_test.go +++ b/context_test.go @@ -3,6 +3,7 @@ package cli import ( "flag" "os" + "strings" "testing" "time" ) @@ -404,10 +405,11 @@ func TestContext_GlobalSet(t *testing.T) { func TestCheckRequiredFlags(t *testing.T) { tdata := []struct { - testCase string - parseInput []string - flags []Flag - expectedAnError bool + testCase string + parseInput []string + flags []Flag + expectedAnError bool + expectedErrorContents []string }{ { testCase: "empty", @@ -423,7 +425,8 @@ func TestCheckRequiredFlags(t *testing.T) { flags: []Flag{ StringFlag{Name: "requiredFlag", Required: true}, }, - expectedAnError: true, + expectedAnError: true, + expectedErrorContents: []string{"requiredFlag"}, }, { testCase: "required_and_present", @@ -460,10 +463,11 @@ func TestCheckRequiredFlags(t *testing.T) { { testCase: "two_required", flags: []Flag{ - StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "requiredFlagOne", Required: true}, StringFlag{Name: "requiredFlagTwo", Required: true}, }, - expectedAnError: true, + expectedAnError: true, + expectedErrorContents: []string{"requiredFlagOne", "requiredFlagTwo"}, }, { testCase: "two_required_and_one_present", @@ -502,6 +506,11 @@ func TestCheckRequiredFlags(t *testing.T) { if !test.expectedAnError && err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } + for _, errString := range test.expectedErrorContents { + if !strings.Contains(err.Error(), errString) { + t.Errorf("expected error %q to contain %q, but it didn't!", err.Error(), errString) + } + } }) } } From 01d5cfab7066912c97eeaf94cbbda8f90fc490f7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 17 Jul 2019 00:20:44 -0700 Subject: [PATCH 32/58] use strings.Join --- context.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/context.go b/context.go index 383749d..91a9575 100644 --- a/context.go +++ b/context.go @@ -308,16 +308,8 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { return fmt.Errorf("Required flag %q not set", missingFlags[0]) } if numberOfMissingFlags >= 2 { - var missingFlagsOutput string - for idx, f := range missingFlags { - // if not the last item, append with a ", " - if idx != numberOfMissingFlags-1 { - missingFlagsOutput = fmt.Sprintf("%s%s, ", missingFlagsOutput, f) - } else { - missingFlagsOutput = fmt.Sprintf("%s%s", missingFlagsOutput, f) - } - } - return fmt.Errorf("Required flags %q not set", missingFlagsOutput) + joinedMissingFlags := strings.Join(missingFlags, ", ") + return fmt.Errorf("Required flags %q not set", joinedMissingFlags) } return nil From 32d84d8e870a7f475c228c3c58f2c879f6a4009e Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Wed, 17 Jul 2019 00:25:13 -0700 Subject: [PATCH 33/58] copy update --- flag.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/flag.go b/flag.go index 71fd729..afb0fed 100644 --- a/flag.go +++ b/flag.go @@ -75,8 +75,8 @@ type Flag interface { GetName() string } -// RequiredFlag is an interface that allows us to return mark flags as required -// it allows flags defined in this library to be marked as required in a backwards compatible fashion +// RequiredFlag is an interface that allows us to mark flags as required +// it allows flags required flags to be backwards compatible with the Flag interface type RequiredFlag interface { Flag From cc1cf8c459c947156bb429ef319f4cf762b1e468 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:09:07 -0700 Subject: [PATCH 34/58] wording shift --- app_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app_test.go b/app_test.go index e838d30..5f32483 100644 --- a/app_test.go +++ b/app_test.go @@ -913,9 +913,9 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, { // expectations: - // - --help input, when a required flag is present, shows the help message - // - --help input, when a required flag is present, explicitly does not error - // - --help input, when a required flag is present, explicitly does not show the flag error message + // - inputing --help, when a required flag is present, shows the help message + // - inputing --help, when a required flag is present, explicitly does not error + // - inputing --help, when a required flag is present, explicitly does not show the flag error message testCase: "help_input_with_required_flag", appRunInput: []string{"command", "--help"}, flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, @@ -925,9 +925,9 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, { // expectations: - // - optional input, when a required flag is present, shows the help message - // - optional input, when a required flag is present, errors - // - optional input, when a required flag is present, shows the flag error message + // - giving optional input, when a required flag is present, shows the help message + // - giving optional input, when a required flag is present, errors + // - giving optional input, when a required flag is present, shows the flag error message testCase: "optional_input_with_required_flag", appRunInput: []string{"command", "--optional", "cats"}, flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, From 300288670fe7713da8ae6e4a449d12e6c911b713 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:20:32 -0700 Subject: [PATCH 35/58] add subcommand --- app_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app_test.go b/app_test.go index 5f32483..28c4afc 100644 --- a/app_test.go +++ b/app_test.go @@ -962,6 +962,18 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // setup - app app := NewApp() app.Flags = test.flags + app.Commands = []Command{ + Command{ + Name: "command", + Flags: test.flags, + Subcommands: []Command{ + Command{ + Name: "subcommand", + Flags: test.flags, + }, + }, + }, + } // logic under test err := app.Run(test.appRunInput) From 2299852c3c3512dafac738a10847da3bb3699b62 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:47:18 -0700 Subject: [PATCH 36/58] cleanup subcommand and specs --- app.go | 4 ++-- app_test.go | 17 +---------------- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/app.go b/app.go index 52c2d85..485c92b 100644 --- a/app.go +++ b/app.go @@ -327,7 +327,6 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { set.SetOutput(ioutil.Discard) err = set.Parse(ctx.Args().Tail()) nerr := normalizeFlags(a.Flags, set) - cerr := checkRequiredFlags(a.Flags, set) context := NewContext(a, set, ctx) if nerr != nil { @@ -341,8 +340,9 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { return nerr } + cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - ShowAppHelp(context) + ShowSubcommandHelp(context) return cerr } diff --git a/app_test.go b/app_test.go index 28c4afc..660d238 100644 --- a/app_test.go +++ b/app_test.go @@ -890,22 +890,13 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { expectedAnError bool expectedFlagErrorPrinter bool }{ - { - // expectations: - // - empty input shows the help message - // - empty input explicitly does not error - testCase: "empty_input", - appRunInput: []string{""}, - expectedHelpPrinter: true, - expectedAnError: false, // explicit false for readability (this is the default value) - }, { // expectations: // - empty input, when a required flag is present, shows the help message // - empty input, when a required flag is present, errors // - empty input, when a required flag is present, show the flag error message testCase: "empty_input_with_required_flag", - appRunInput: []string{""}, + appRunInput: []string{"command"}, flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedHelpPrinter: true, expectedAnError: true, @@ -966,12 +957,6 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { Command{ Name: "command", Flags: test.flags, - Subcommands: []Command{ - Command{ - Name: "subcommand", - Flags: test.flags, - }, - }, }, } From 19140e1fb52f458727a3c718f82fb93861d5849c Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:48:09 -0700 Subject: [PATCH 37/58] show errors --- app.go | 1 + 1 file changed, 1 insertion(+) diff --git a/app.go b/app.go index 485c92b..44d6645 100644 --- a/app.go +++ b/app.go @@ -342,6 +342,7 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { + showFlagError(a.Writer, cerr) ShowSubcommandHelp(context) return cerr } From d8985dc6d56ac75b35f0422d8efbc04814bf17f3 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:51:16 -0700 Subject: [PATCH 38/58] reduce diff --- app.go | 1 - 1 file changed, 1 deletion(-) diff --git a/app.go b/app.go index 44d6645..6fc0a42 100644 --- a/app.go +++ b/app.go @@ -201,7 +201,6 @@ func (a *App) Run(arguments []string) (err error) { err = set.Parse(arguments[1:]) nerr := normalizeFlags(a.Flags, set) context := NewContext(a, set, nil) - if nerr != nil { fmt.Fprintln(a.Writer, nerr) ShowAppHelp(context) From 7ce0af189ed431005f47e583a63648ea9a0a99ea Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 18 Jul 2019 00:52:24 -0700 Subject: [PATCH 39/58] remove unused code --- flag.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/flag.go b/flag.go index afb0fed..d98c808 100644 --- a/flag.go +++ b/flag.go @@ -649,18 +649,6 @@ func withEnvHint(envVar, str string) string { return str + envText } -func withRequiredHint(isRequired bool, str string) string { - if isRequired { - return str + " (required)" - } - - return str -} - -func withHints(envVar string, isRequired bool, str string) string { - return withRequiredHint(isRequired, withEnvHint(envVar, str)) -} - func withFileHint(filePath, str string) string { fileText := "" if filePath != "" { From 23f09ac1e82395dc1a70c36d649ab03929e32d79 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 28 Jul 2019 22:19:35 -0700 Subject: [PATCH 40/58] cleanup tests, check required flags in more places --- app.go | 14 +++++------ app_test.go | 70 ++++++++++++++++++----------------------------------- command.go | 7 ++++++ 3 files changed, 38 insertions(+), 53 deletions(-) diff --git a/app.go b/app.go index 6fc0a42..c286327 100644 --- a/app.go +++ b/app.go @@ -339,13 +339,6 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { return nerr } - cerr := checkRequiredFlags(a.Flags, set) - if cerr != nil { - showFlagError(a.Writer, cerr) - ShowSubcommandHelp(context) - return cerr - } - if checkCompletions(context) { return nil } @@ -371,6 +364,13 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } } + cerr := checkRequiredFlags(a.Flags, set) + if cerr != nil { + showFlagError(a.Writer, cerr) + ShowSubcommandHelp(context) + return cerr + } + if a.After != nil { defer func() { afterErr := a.After(context) diff --git a/app_test.go b/app_test.go index 660d238..cd4b1ef 100644 --- a/app_test.go +++ b/app_test.go @@ -877,54 +877,38 @@ func TestAppNoHelpFlag(t *testing.T) { } } -// TestRequiredFlagAppRunBehavior tests the app-wide behavior of required flags -// and how they interact with the error response and help messages. -// A different test (`TestCheckRequiredFlags`) exists for the more fine grain -// behavior of required flags. func TestRequiredFlagAppRunBehavior(t *testing.T) { tdata := []struct { - testCase string - flags []Flag - appRunInput []string - expectedHelpPrinter bool - expectedAnError bool - expectedFlagErrorPrinter bool + testCase string + flags []Flag + appRunInput []string + expectedAnError bool }{ { // expectations: // - empty input, when a required flag is present, shows the help message - // - empty input, when a required flag is present, errors - // - empty input, when a required flag is present, show the flag error message - testCase: "empty_input_with_required_flag", - appRunInput: []string{"command"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, - expectedHelpPrinter: true, - expectedAnError: true, - expectedFlagErrorPrinter: true, + // - empty input, when a required flag is present, errors and shows the flag error message + testCase: "empty_input_with_required_flag", + appRunInput: []string{"command"}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + expectedAnError: true, }, { // expectations: // - inputing --help, when a required flag is present, shows the help message - // - inputing --help, when a required flag is present, explicitly does not error - // - inputing --help, when a required flag is present, explicitly does not show the flag error message - testCase: "help_input_with_required_flag", - appRunInput: []string{"command", "--help"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, - expectedHelpPrinter: true, - expectedAnError: false, // explicit false for readability (this is the default value) - expectedFlagErrorPrinter: false, // explicit false for readability (this is the default value) + // - inputing --help, when a required flag is present, does not error + testCase: "help_input_with_required_flag", + appRunInput: []string{"command", "--help"}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, { // expectations: // - giving optional input, when a required flag is present, shows the help message - // - giving optional input, when a required flag is present, errors - // - giving optional input, when a required flag is present, shows the flag error message - testCase: "optional_input_with_required_flag", - appRunInput: []string{"command", "--optional", "cats"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, - expectedHelpPrinter: true, - expectedAnError: true, - expectedFlagErrorPrinter: true, + // - giving optional input, when a required flag is present, errors and shows the flag error message + testCase: "optional_input_with_required_flag", + appRunInput: []string{"command", "--optional", "cats"}, + flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, + expectedAnError: true, }, } for _, test := range tdata { @@ -935,9 +919,9 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { showFlagError = oldShowError }() // setup - mock showFlagError - var showErrorWasCalled = false + var showFlagErrorWasCalled = false showFlagError = func(writer io.Writer, err ...interface{}) (int, error) { - showErrorWasCalled = true + showFlagErrorWasCalled = true return 0, nil } // setup - undo HelpPrinter mock when finished @@ -953,26 +937,20 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // setup - app app := NewApp() app.Flags = test.flags - app.Commands = []Command{ - Command{ - Name: "command", - Flags: test.flags, - }, - } // logic under test err := app.Run(test.appRunInput) // assertions - if test.expectedHelpPrinter && helpPrinterWasCalled == false { + if helpPrinterWasCalled == false { t.Errorf("HelpPrinter expected to be called, but was not") } - if test.expectedFlagErrorPrinter && showErrorWasCalled == false { - t.Errorf("showFlagError expected to be called, but was not") - } if test.expectedAnError && err == nil { t.Errorf("expected an error, but there was none") } + if test.expectedAnError && showFlagErrorWasCalled == false { + t.Errorf("showFlagError expected to be called, but was not") + } if !test.expectedAnError && err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } diff --git a/command.go b/command.go index 3d44404..a6b19b5 100644 --- a/command.go +++ b/command.go @@ -135,6 +135,13 @@ func (c Command) Run(ctx *Context) (err error) { return nil } + cerr := checkRequiredFlags(c.Flags, set) + if cerr != nil { + showFlagError(context.App.Writer, cerr) + ShowCommandHelp(context, c.Name) + return cerr + } + if c.After != nil { defer func() { afterErr := c.After(context) From 9ec594d5290b846de59b0bc350849b848cabfbd7 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 28 Jul 2019 22:34:07 -0700 Subject: [PATCH 41/58] reset generated flags changes --- flag_generated.go | 78 --------------------------------------------- generate-flag-types | 6 ---- 2 files changed, 84 deletions(-) diff --git a/flag_generated.go b/flag_generated.go index a3e4d6e..001576c 100644 --- a/flag_generated.go +++ b/flag_generated.go @@ -14,7 +14,6 @@ type BoolFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Destination *bool } @@ -30,11 +29,6 @@ func (f BoolFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f BoolFlag) IsRequired() bool { - return f.Required -} - // Bool looks up the value of a local BoolFlag, returns // false if not found func (c *Context) Bool(name string) bool { @@ -68,7 +62,6 @@ type BoolTFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Destination *bool } @@ -84,11 +77,6 @@ func (f BoolTFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f BoolTFlag) IsRequired() bool { - return f.Required -} - // BoolT looks up the value of a local BoolTFlag, returns // false if not found func (c *Context) BoolT(name string) bool { @@ -122,7 +110,6 @@ type DurationFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value time.Duration Destination *time.Duration @@ -139,11 +126,6 @@ func (f DurationFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f DurationFlag) IsRequired() bool { - return f.Required -} - // Duration looks up the value of a local DurationFlag, returns // 0 if not found func (c *Context) Duration(name string) time.Duration { @@ -177,7 +159,6 @@ type Float64Flag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value float64 Destination *float64 @@ -194,11 +175,6 @@ func (f Float64Flag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f Float64Flag) IsRequired() bool { - return f.Required -} - // Float64 looks up the value of a local Float64Flag, returns // 0 if not found func (c *Context) Float64(name string) float64 { @@ -232,7 +208,6 @@ type GenericFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value Generic } @@ -248,11 +223,6 @@ func (f GenericFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f GenericFlag) IsRequired() bool { - return f.Required -} - // Generic looks up the value of a local GenericFlag, returns // nil if not found func (c *Context) Generic(name string) interface{} { @@ -286,7 +256,6 @@ type Int64Flag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value int64 Destination *int64 @@ -303,11 +272,6 @@ func (f Int64Flag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f Int64Flag) IsRequired() bool { - return f.Required -} - // Int64 looks up the value of a local Int64Flag, returns // 0 if not found func (c *Context) Int64(name string) int64 { @@ -341,7 +305,6 @@ type IntFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value int Destination *int @@ -358,11 +321,6 @@ func (f IntFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f IntFlag) IsRequired() bool { - return f.Required -} - // Int looks up the value of a local IntFlag, returns // 0 if not found func (c *Context) Int(name string) int { @@ -396,7 +354,6 @@ type IntSliceFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value *IntSlice } @@ -412,11 +369,6 @@ func (f IntSliceFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f IntSliceFlag) IsRequired() bool { - return f.Required -} - // IntSlice looks up the value of a local IntSliceFlag, returns // nil if not found func (c *Context) IntSlice(name string) []int { @@ -450,7 +402,6 @@ type Int64SliceFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value *Int64Slice } @@ -466,11 +417,6 @@ func (f Int64SliceFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f Int64SliceFlag) IsRequired() bool { - return f.Required -} - // Int64Slice looks up the value of a local Int64SliceFlag, returns // nil if not found func (c *Context) Int64Slice(name string) []int64 { @@ -504,7 +450,6 @@ type StringFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value string Destination *string @@ -521,11 +466,6 @@ func (f StringFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f StringFlag) IsRequired() bool { - return f.Required -} - // String looks up the value of a local StringFlag, returns // "" if not found func (c *Context) String(name string) string { @@ -559,7 +499,6 @@ type StringSliceFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value *StringSlice } @@ -575,11 +514,6 @@ func (f StringSliceFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f StringSliceFlag) IsRequired() bool { - return f.Required -} - // StringSlice looks up the value of a local StringSliceFlag, returns // nil if not found func (c *Context) StringSlice(name string) []string { @@ -613,7 +547,6 @@ type Uint64Flag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value uint64 Destination *uint64 @@ -630,11 +563,6 @@ func (f Uint64Flag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f Uint64Flag) IsRequired() bool { - return f.Required -} - // Uint64 looks up the value of a local Uint64Flag, returns // 0 if not found func (c *Context) Uint64(name string) uint64 { @@ -668,7 +596,6 @@ type UintFlag struct { Usage string EnvVar string FilePath string - Required bool Hidden bool Value uint Destination *uint @@ -685,11 +612,6 @@ func (f UintFlag) GetName() string { return f.Name } -// IsRequired returns the whether or not the flag is required -func (f UintFlag) IsRequired() bool { - return f.Required -} - // Uint looks up the value of a local UintFlag, returns // 0 if not found func (c *Context) Uint(name string) uint { diff --git a/generate-flag-types b/generate-flag-types index e0b5aa1..1358857 100755 --- a/generate-flag-types +++ b/generate-flag-types @@ -143,7 +143,6 @@ def _write_cli_flag_types(outfile, types): Usage string EnvVar string FilePath string - Required bool Hidden bool """.format(**typedef)) @@ -171,11 +170,6 @@ def _write_cli_flag_types(outfile, types): return f.Name }} - // IsRequired returns the whether or not the flag is required - func (f {name}Flag) IsRequired() bool {{ - return f.Required - }} - // {name} looks up the value of a local {name}Flag, returns // {context_default} if not found func (c *Context) {name}(name string) {context_type} {{ From 386b379d1950e8939c8a3dbba0335cf79903f421 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Sun, 28 Jul 2019 22:45:43 -0700 Subject: [PATCH 42/58] Revert "reset generated flags changes" This reverts commit 9ec594d5290b846de59b0bc350849b848cabfbd7. --- flag_generated.go | 78 +++++++++++++++++++++++++++++++++++++++++++++ generate-flag-types | 6 ++++ 2 files changed, 84 insertions(+) diff --git a/flag_generated.go b/flag_generated.go index 001576c..a3e4d6e 100644 --- a/flag_generated.go +++ b/flag_generated.go @@ -14,6 +14,7 @@ type BoolFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Destination *bool } @@ -29,6 +30,11 @@ func (f BoolFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f BoolFlag) IsRequired() bool { + return f.Required +} + // Bool looks up the value of a local BoolFlag, returns // false if not found func (c *Context) Bool(name string) bool { @@ -62,6 +68,7 @@ type BoolTFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Destination *bool } @@ -77,6 +84,11 @@ func (f BoolTFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f BoolTFlag) IsRequired() bool { + return f.Required +} + // BoolT looks up the value of a local BoolTFlag, returns // false if not found func (c *Context) BoolT(name string) bool { @@ -110,6 +122,7 @@ type DurationFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value time.Duration Destination *time.Duration @@ -126,6 +139,11 @@ func (f DurationFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f DurationFlag) IsRequired() bool { + return f.Required +} + // Duration looks up the value of a local DurationFlag, returns // 0 if not found func (c *Context) Duration(name string) time.Duration { @@ -159,6 +177,7 @@ type Float64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value float64 Destination *float64 @@ -175,6 +194,11 @@ func (f Float64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Float64Flag) IsRequired() bool { + return f.Required +} + // Float64 looks up the value of a local Float64Flag, returns // 0 if not found func (c *Context) Float64(name string) float64 { @@ -208,6 +232,7 @@ type GenericFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value Generic } @@ -223,6 +248,11 @@ func (f GenericFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f GenericFlag) IsRequired() bool { + return f.Required +} + // Generic looks up the value of a local GenericFlag, returns // nil if not found func (c *Context) Generic(name string) interface{} { @@ -256,6 +286,7 @@ type Int64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value int64 Destination *int64 @@ -272,6 +303,11 @@ func (f Int64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Int64Flag) IsRequired() bool { + return f.Required +} + // Int64 looks up the value of a local Int64Flag, returns // 0 if not found func (c *Context) Int64(name string) int64 { @@ -305,6 +341,7 @@ type IntFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value int Destination *int @@ -321,6 +358,11 @@ func (f IntFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f IntFlag) IsRequired() bool { + return f.Required +} + // Int looks up the value of a local IntFlag, returns // 0 if not found func (c *Context) Int(name string) int { @@ -354,6 +396,7 @@ type IntSliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *IntSlice } @@ -369,6 +412,11 @@ func (f IntSliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f IntSliceFlag) IsRequired() bool { + return f.Required +} + // IntSlice looks up the value of a local IntSliceFlag, returns // nil if not found func (c *Context) IntSlice(name string) []int { @@ -402,6 +450,7 @@ type Int64SliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *Int64Slice } @@ -417,6 +466,11 @@ func (f Int64SliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Int64SliceFlag) IsRequired() bool { + return f.Required +} + // Int64Slice looks up the value of a local Int64SliceFlag, returns // nil if not found func (c *Context) Int64Slice(name string) []int64 { @@ -450,6 +504,7 @@ type StringFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value string Destination *string @@ -466,6 +521,11 @@ func (f StringFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f StringFlag) IsRequired() bool { + return f.Required +} + // String looks up the value of a local StringFlag, returns // "" if not found func (c *Context) String(name string) string { @@ -499,6 +559,7 @@ type StringSliceFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value *StringSlice } @@ -514,6 +575,11 @@ func (f StringSliceFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f StringSliceFlag) IsRequired() bool { + return f.Required +} + // StringSlice looks up the value of a local StringSliceFlag, returns // nil if not found func (c *Context) StringSlice(name string) []string { @@ -547,6 +613,7 @@ type Uint64Flag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value uint64 Destination *uint64 @@ -563,6 +630,11 @@ func (f Uint64Flag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f Uint64Flag) IsRequired() bool { + return f.Required +} + // Uint64 looks up the value of a local Uint64Flag, returns // 0 if not found func (c *Context) Uint64(name string) uint64 { @@ -596,6 +668,7 @@ type UintFlag struct { Usage string EnvVar string FilePath string + Required bool Hidden bool Value uint Destination *uint @@ -612,6 +685,11 @@ func (f UintFlag) GetName() string { return f.Name } +// IsRequired returns the whether or not the flag is required +func (f UintFlag) IsRequired() bool { + return f.Required +} + // Uint looks up the value of a local UintFlag, returns // 0 if not found func (c *Context) Uint(name string) uint { diff --git a/generate-flag-types b/generate-flag-types index 1358857..e0b5aa1 100755 --- a/generate-flag-types +++ b/generate-flag-types @@ -143,6 +143,7 @@ def _write_cli_flag_types(outfile, types): Usage string EnvVar string FilePath string + Required bool Hidden bool """.format(**typedef)) @@ -170,6 +171,11 @@ def _write_cli_flag_types(outfile, types): return f.Name }} + // IsRequired returns the whether or not the flag is required + func (f {name}Flag) IsRequired() bool {{ + return f.Required + }} + // {name} looks up the value of a local {name}Flag, returns // {context_default} if not found func (c *Context) {name}(name string) {context_type} {{ From 9438aba3b89e7053070ef277121a14e5fb95947e Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 19:54:57 -0700 Subject: [PATCH 43/58] remove showFlagError, we can use the help printer assertion to accomplish the same goal --- app.go | 6 ++---- app_test.go | 14 -------------- command.go | 4 ++-- 3 files changed, 4 insertions(+), 20 deletions(-) diff --git a/app.go b/app.go index c286327..6557044 100644 --- a/app.go +++ b/app.go @@ -23,8 +23,6 @@ var ( errInvalidActionType = NewExitError("ERROR invalid Action type. "+ fmt.Sprintf("Must be `func(*Context`)` or `func(*Context) error). %s", contactSysadmin)+ fmt.Sprintf("See %s", appActionDeprecationURL), 2) - - showFlagError printerFunc = fmt.Fprintln ) // App is the main structure of a cli application. It is recommended that @@ -235,7 +233,7 @@ func (a *App) Run(arguments []string) (err error) { cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - showFlagError(a.Writer, cerr) + fmt.Fprintln(a.Writer, cerr) ShowAppHelp(context) return cerr } @@ -366,7 +364,7 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - showFlagError(a.Writer, cerr) + fmt.Fprintln(a.Writer, cerr) ShowSubcommandHelp(context) return cerr } diff --git a/app_test.go b/app_test.go index cd4b1ef..36f8a72 100644 --- a/app_test.go +++ b/app_test.go @@ -913,17 +913,6 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { - // setup - undo showFlagError mock when finished - oldShowError := showFlagError - defer func() { - showFlagError = oldShowError - }() - // setup - mock showFlagError - var showFlagErrorWasCalled = false - showFlagError = func(writer io.Writer, err ...interface{}) (int, error) { - showFlagErrorWasCalled = true - return 0, nil - } // setup - undo HelpPrinter mock when finished oldPrinter := HelpPrinter defer func() { @@ -948,9 +937,6 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { if test.expectedAnError && err == nil { t.Errorf("expected an error, but there was none") } - if test.expectedAnError && showFlagErrorWasCalled == false { - t.Errorf("showFlagError expected to be called, but was not") - } if !test.expectedAnError && err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } diff --git a/command.go b/command.go index a6b19b5..fdec3c9 100644 --- a/command.go +++ b/command.go @@ -137,7 +137,7 @@ func (c Command) Run(ctx *Context) (err error) { cerr := checkRequiredFlags(c.Flags, set) if cerr != nil { - showFlagError(context.App.Writer, cerr) + fmt.Fprintln(context.App.Writer, cerr) ShowCommandHelp(context, c.Name) return cerr } @@ -273,7 +273,7 @@ func reorderArgs(args []string) []string { } func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string { - allCharsFlags := func (s string) bool { + allCharsFlags := func(s string) bool { for i := range s { f := set.Lookup(string(s[i])) if f == nil { From 714a73f028fa5a3c5b6512bf5e55b94be388de8f Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 19:57:14 -0700 Subject: [PATCH 44/58] remove unused thing --- app.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/app.go b/app.go index 6557044..80989aa 100644 --- a/app.go +++ b/app.go @@ -10,9 +10,6 @@ import ( "time" ) -// printerFunc is the function signature for fmt.Fprintln -type printerFunc func(io.Writer, ...interface{}) (int, error) - var ( changeLogURL = "https://github.com/urfave/cli/blob/master/CHANGELOG.md" appActionDeprecationURL = fmt.Sprintf("%s#deprecated-cli-app-action-signature", changeLogURL) From 95d3a8624d8aa6661831d0009550b602458fcb4d Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 20:27:51 -0700 Subject: [PATCH 45/58] update test to reflect app flag usage --- app_test.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/app_test.go b/app_test.go index 36f8a72..bbd1426 100644 --- a/app_test.go +++ b/app_test.go @@ -880,7 +880,7 @@ func TestAppNoHelpFlag(t *testing.T) { func TestRequiredFlagAppRunBehavior(t *testing.T) { tdata := []struct { testCase string - flags []Flag + appFlags []Flag appRunInput []string expectedAnError bool }{ @@ -889,8 +889,8 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // - empty input, when a required flag is present, shows the help message // - empty input, when a required flag is present, errors and shows the flag error message testCase: "empty_input_with_required_flag", - appRunInput: []string{"command"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + appRunInput: []string{"myCLI"}, + appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedAnError: true, }, { @@ -898,18 +898,23 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // - inputing --help, when a required flag is present, shows the help message // - inputing --help, when a required flag is present, does not error testCase: "help_input_with_required_flag", - appRunInput: []string{"command", "--help"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + appRunInput: []string{"myCLI", "--help"}, + appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, { // expectations: // - giving optional input, when a required flag is present, shows the help message // - giving optional input, when a required flag is present, errors and shows the flag error message testCase: "optional_input_with_required_flag", - appRunInput: []string{"command", "--optional", "cats"}, - flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, + appRunInput: []string{"myCLI", "--optional", "cats"}, + appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, expectedAnError: true, }, + { + testCase: "required_flag_input", + appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, + appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }, } for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { @@ -925,7 +930,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } // setup - app app := NewApp() - app.Flags = test.flags + app.Flags = test.appFlags // logic under test err := app.Run(test.appRunInput) From 7b9e16b6b5255803ea279fe1ee0e41973f49a42e Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 20:30:43 -0700 Subject: [PATCH 46/58] update test names --- app_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app_test.go b/app_test.go index bbd1426..6f36ef9 100644 --- a/app_test.go +++ b/app_test.go @@ -882,13 +882,14 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { testCase string appFlags []Flag appRunInput []string + appCommands []Command expectedAnError bool }{ { // expectations: // - empty input, when a required flag is present, shows the help message // - empty input, when a required flag is present, errors and shows the flag error message - testCase: "empty_input_with_required_flag", + testCase: "error_case_empty_input_with_required_flag", appRunInput: []string{"myCLI"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedAnError: true, @@ -897,7 +898,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // expectations: // - inputing --help, when a required flag is present, shows the help message // - inputing --help, when a required flag is present, does not error - testCase: "help_input_with_required_flag", + testCase: "valid_case_help_input_with_required_flag", appRunInput: []string{"myCLI", "--help"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, @@ -905,13 +906,13 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // expectations: // - giving optional input, when a required flag is present, shows the help message // - giving optional input, when a required flag is present, errors and shows the flag error message - testCase: "optional_input_with_required_flag", + testCase: "error_case_optional_input_with_required_flag", appRunInput: []string{"myCLI", "--optional", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, expectedAnError: true, }, { - testCase: "required_flag_input", + testCase: "valid_case_required_flag_input", appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, @@ -931,6 +932,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // setup - app app := NewApp() app.Flags = test.appFlags + app.Commands = test.appCommands // logic under test err := app.Run(test.appRunInput) From 3d6eec825ac768894a385ca3c3156a7905c27ce3 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 20:35:23 -0700 Subject: [PATCH 47/58] add test cases --- app_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/app_test.go b/app_test.go index 6f36ef9..63f0fc4 100644 --- a/app_test.go +++ b/app_test.go @@ -916,6 +916,25 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, + { + testCase: "valid_case_required_flag_input_command", + appRunInput: []string{"myCLI", "myCommand", "--requiredFlag", "cats"}, + appCommands: []Command{Command{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + }, + { + testCase: "valid_case_required_flag_input_subcommand", + appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--requiredFlag", "cats"}, + appCommands: []Command{Command{ + Name: "myCommand", + Subcommands: []Command{Command{ + Name: "mySubCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + }}, + }, } for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { From 595382c50970039261f765043aee4c647aeccbd5 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 20:39:37 -0700 Subject: [PATCH 48/58] expand test cases --- app_test.go | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/app_test.go b/app_test.go index 63f0fc4..f785a44 100644 --- a/app_test.go +++ b/app_test.go @@ -885,32 +885,50 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { appCommands []Command expectedAnError bool }{ + // expectations: + // - empty input, when a required flag is present, errors and shows the flag error message { - // expectations: - // - empty input, when a required flag is present, shows the help message - // - empty input, when a required flag is present, errors and shows the flag error message testCase: "error_case_empty_input_with_required_flag", appRunInput: []string{"myCLI"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedAnError: true, }, + // expectations: + // - inputing --help, when a required flag is present, does not error { - // expectations: - // - inputing --help, when a required flag is present, shows the help message - // - inputing --help, when a required flag is present, does not error testCase: "valid_case_help_input_with_required_flag", appRunInput: []string{"myCLI", "--help"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, { - // expectations: - // - giving optional input, when a required flag is present, shows the help message - // - giving optional input, when a required flag is present, errors and shows the flag error message + testCase: "valid_case_help_input_with_required_flag_command", + appRunInput: []string{"myCLI", "myCommand", "--help"}, + appCommands: []Command{Command{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + }, + { + testCase: "valid_case_help_input_with_required_flag_subcommand", + appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--help"}, + appCommands: []Command{Command{ + Name: "myCommand", + Subcommands: []Command{Command{ + Name: "mySubCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + }}, + }, + // expectations: + // - giving optional input, when a required flag is present, shows the help message + // - giving optional input, when a required flag is present, errors and shows the flag error message + { testCase: "error_case_optional_input_with_required_flag", appRunInput: []string{"myCLI", "--optional", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, expectedAnError: true, }, + // valid input cases { testCase: "valid_case_required_flag_input", appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, From d4740d10d0cbde53a8e3132a0964464b2b50fc0b Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 20:58:08 -0700 Subject: [PATCH 49/58] more test cases --- app_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app_test.go b/app_test.go index f785a44..b469644 100644 --- a/app_test.go +++ b/app_test.go @@ -893,6 +893,27 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedAnError: true, }, + { + testCase: "error_case_empty_input_with_required_flag", + appRunInput: []string{"myCLI", "myCommand"}, + appCommands: []Command{Command{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + expectedAnError: true, + }, + { + testCase: "error_case_empty_input_with_required_flag", + appRunInput: []string{"myCLI", "myCommand", "mySubCommand"}, + appCommands: []Command{Command{ + Name: "myCommand", + Subcommands: []Command{Command{ + Name: "mySubCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, + }}, + }}, + expectedAnError: true, + }, // expectations: // - inputing --help, when a required flag is present, does not error { From 78db152323afb7934f9f0dd207eeaf34147bb300 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 21:35:15 -0700 Subject: [PATCH 50/58] add typed error assertions --- app_test.go | 3 +++ context.go | 33 +++++++++++++++++++++++++-------- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/app_test.go b/app_test.go index b469644..1e42f12 100644 --- a/app_test.go +++ b/app_test.go @@ -1002,6 +1002,9 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { if test.expectedAnError && err == nil { t.Errorf("expected an error, but there was none") } + if _, ok := err.(requiredFlagsErr); test.expectedAnError && !ok { + t.Errorf("expected a requiredFlagsErr, but got: %s", err) + } if !test.expectedAnError && err != nil { t.Errorf("did not expected an error, but there was one: %s", err) } diff --git a/context.go b/context.go index 91a9575..498fd2a 100644 --- a/context.go +++ b/context.go @@ -287,7 +287,29 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error { return nil } -func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { +type requiredFlagsErr interface { + error + getMissingFlags() []string +} + +type errRequiredFlags struct { + missingFlags []string +} + +func (e *errRequiredFlags) Error() string { + numberOfMissingFlags := len(e.missingFlags) + if numberOfMissingFlags == 1 { + return fmt.Sprintf("Required flag %q not set", e.missingFlags[0]) + } + joinedMissingFlags := strings.Join(e.missingFlags, ", ") + return fmt.Sprintf("Required flags %q not set", joinedMissingFlags) +} + +func (e *errRequiredFlags) getMissingFlags() []string { + return e.missingFlags +} + +func checkRequiredFlags(flags []Flag, set *flag.FlagSet) requiredFlagsErr { visited := make(map[string]bool) set.Visit(func(f *flag.Flag) { visited[f.Name] = true @@ -303,13 +325,8 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) error { } } - numberOfMissingFlags := len(missingFlags) - if numberOfMissingFlags == 1 { - return fmt.Errorf("Required flag %q not set", missingFlags[0]) - } - if numberOfMissingFlags >= 2 { - joinedMissingFlags := strings.Join(missingFlags, ", ") - return fmt.Errorf("Required flags %q not set", joinedMissingFlags) + if len(missingFlags) != 0 { + return &errRequiredFlags{missingFlags: missingFlags} } return nil From 45f2b3d8e71e11822cf591f1c370f8587726c425 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 21:45:11 -0700 Subject: [PATCH 51/58] more test cases --- app_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app_test.go b/app_test.go index 1e42f12..465d3ce 100644 --- a/app_test.go +++ b/app_test.go @@ -949,6 +949,27 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, expectedAnError: true, }, + { + testCase: "error_case_optional_input_with_required_flag_command", + appRunInput: []string{"myCLI", "myCommand", "--optional", "cats"}, + appCommands: []Command{Command{ + Name: "myCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, + }}, + expectedAnError: true, + }, + { + testCase: "error_case_optional_input_with_required_flag_subcommand", + appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--optional", "cats"}, + appCommands: []Command{Command{ + Name: "myCommand", + Subcommands: []Command{Command{ + Name: "mySubCommand", + Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, + }}, + }}, + expectedAnError: true, + }, // valid input cases { testCase: "valid_case_required_flag_input", From ef9acb4a3b846728c98844f7f92964ae2a79f259 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 21:46:56 -0700 Subject: [PATCH 52/58] rename cases --- app_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/app_test.go b/app_test.go index 465d3ce..854745f 100644 --- a/app_test.go +++ b/app_test.go @@ -888,13 +888,13 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // expectations: // - empty input, when a required flag is present, errors and shows the flag error message { - testCase: "error_case_empty_input_with_required_flag", + testCase: "error_case_empty_input_with_required_flag_on_app", appRunInput: []string{"myCLI"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, expectedAnError: true, }, { - testCase: "error_case_empty_input_with_required_flag", + testCase: "error_case_empty_input_with_required_flag_on_command", appRunInput: []string{"myCLI", "myCommand"}, appCommands: []Command{Command{ Name: "myCommand", @@ -903,7 +903,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { expectedAnError: true, }, { - testCase: "error_case_empty_input_with_required_flag", + testCase: "error_case_empty_input_with_required_flag_on_subcommand", appRunInput: []string{"myCLI", "myCommand", "mySubCommand"}, appCommands: []Command{Command{ Name: "myCommand", @@ -917,12 +917,12 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // expectations: // - inputing --help, when a required flag is present, does not error { - testCase: "valid_case_help_input_with_required_flag", + testCase: "valid_case_help_input_with_required_flag_on_app", appRunInput: []string{"myCLI", "--help"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, { - testCase: "valid_case_help_input_with_required_flag_command", + testCase: "valid_case_help_input_with_required_flag_on_command", appRunInput: []string{"myCLI", "myCommand", "--help"}, appCommands: []Command{Command{ Name: "myCommand", @@ -930,7 +930,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }}, }, { - testCase: "valid_case_help_input_with_required_flag_subcommand", + testCase: "valid_case_help_input_with_required_flag_on_subcommand", appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--help"}, appCommands: []Command{Command{ Name: "myCommand", @@ -944,13 +944,13 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { // - giving optional input, when a required flag is present, shows the help message // - giving optional input, when a required flag is present, errors and shows the flag error message { - testCase: "error_case_optional_input_with_required_flag", + testCase: "error_case_optional_input_with_required_flag_on_app", appRunInput: []string{"myCLI", "--optional", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}}, expectedAnError: true, }, { - testCase: "error_case_optional_input_with_required_flag_command", + testCase: "error_case_optional_input_with_required_flag_on_command", appRunInput: []string{"myCLI", "myCommand", "--optional", "cats"}, appCommands: []Command{Command{ Name: "myCommand", @@ -959,7 +959,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { expectedAnError: true, }, { - testCase: "error_case_optional_input_with_required_flag_subcommand", + testCase: "error_case_optional_input_with_required_flag_on_subcommand", appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--optional", "cats"}, appCommands: []Command{Command{ Name: "myCommand", @@ -972,12 +972,12 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }, // valid input cases { - testCase: "valid_case_required_flag_input", + testCase: "valid_case_required_flag_input_on_app", appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, appFlags: []Flag{StringFlag{Name: "requiredFlag", Required: true}}, }, { - testCase: "valid_case_required_flag_input_command", + testCase: "valid_case_required_flag_input_on_command", appRunInput: []string{"myCLI", "myCommand", "--requiredFlag", "cats"}, appCommands: []Command{Command{ Name: "myCommand", @@ -985,7 +985,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }}, }, { - testCase: "valid_case_required_flag_input_subcommand", + testCase: "valid_case_required_flag_input_on_subcommand", appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--requiredFlag", "cats"}, appCommands: []Command{Command{ Name: "myCommand", From fdd4d106912b363ccffa03eec51a56dd3a6a822b Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 21:48:52 -0700 Subject: [PATCH 53/58] update comments --- app_test.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app_test.go b/app_test.go index 854745f..658ca21 100644 --- a/app_test.go +++ b/app_test.go @@ -885,8 +885,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { appCommands []Command expectedAnError bool }{ - // expectations: - // - empty input, when a required flag is present, errors and shows the flag error message + // assertion: empty input, when a required flag is present, errors { testCase: "error_case_empty_input_with_required_flag_on_app", appRunInput: []string{"myCLI"}, @@ -914,8 +913,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }}, expectedAnError: true, }, - // expectations: - // - inputing --help, when a required flag is present, does not error + // assertion: inputing --help, when a required flag is present, does not error { testCase: "valid_case_help_input_with_required_flag_on_app", appRunInput: []string{"myCLI", "--help"}, @@ -940,9 +938,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }}, }}, }, - // expectations: - // - giving optional input, when a required flag is present, shows the help message - // - giving optional input, when a required flag is present, errors and shows the flag error message + // assertion: giving optional input, when a required flag is present, errors { testCase: "error_case_optional_input_with_required_flag_on_app", appRunInput: []string{"myCLI", "--optional", "cats"}, @@ -970,7 +966,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { }}, expectedAnError: true, }, - // valid input cases + // assertion: when a required flag is present, inputting that required flag does not error { testCase: "valid_case_required_flag_input_on_app", appRunInput: []string{"myCLI", "--requiredFlag", "cats"}, From f21b22dd904b638518d9ea321d718f219bd6593c Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 22:10:18 -0700 Subject: [PATCH 54/58] cleanup some issues with error display --- app.go | 2 -- command.go | 1 - context.go | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/app.go b/app.go index 80989aa..051c801 100644 --- a/app.go +++ b/app.go @@ -230,7 +230,6 @@ func (a *App) Run(arguments []string) (err error) { cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - fmt.Fprintln(a.Writer, cerr) ShowAppHelp(context) return cerr } @@ -361,7 +360,6 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { cerr := checkRequiredFlags(a.Flags, set) if cerr != nil { - fmt.Fprintln(a.Writer, cerr) ShowSubcommandHelp(context) return cerr } diff --git a/command.go b/command.go index fdec3c9..cbf06bb 100644 --- a/command.go +++ b/command.go @@ -137,7 +137,6 @@ func (c Command) Run(ctx *Context) (err error) { cerr := checkRequiredFlags(c.Flags, set) if cerr != nil { - fmt.Fprintln(context.App.Writer, cerr) ShowCommandHelp(context, c.Name) return cerr } diff --git a/context.go b/context.go index 498fd2a..8af3264 100644 --- a/context.go +++ b/context.go @@ -320,7 +320,7 @@ func checkRequiredFlags(flags []Flag, set *flag.FlagSet) requiredFlagsErr { if rf, ok := f.(RequiredFlag); ok && rf.IsRequired() { key := strings.Split(f.GetName(), ",")[0] if !visited[key] { - missingFlags = append(missingFlags, f.GetName()) + missingFlags = append(missingFlags, key) } } } From 38f9e1622d2d4a5e1a86afd8f8f9d6cbf0157816 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 22:52:21 -0700 Subject: [PATCH 55/58] add environment variable support :tada: --- app.go | 4 ++-- command.go | 2 +- context.go | 9 ++------- context_test.go | 4 +++- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/app.go b/app.go index 051c801..9ed492f 100644 --- a/app.go +++ b/app.go @@ -228,7 +228,7 @@ func (a *App) Run(arguments []string) (err error) { return nil } - cerr := checkRequiredFlags(a.Flags, set) + cerr := checkRequiredFlags(a.Flags, context) if cerr != nil { ShowAppHelp(context) return cerr @@ -358,7 +358,7 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) { } } - cerr := checkRequiredFlags(a.Flags, set) + cerr := checkRequiredFlags(a.Flags, context) if cerr != nil { ShowSubcommandHelp(context) return cerr diff --git a/command.go b/command.go index cbf06bb..e3b57db 100644 --- a/command.go +++ b/command.go @@ -135,7 +135,7 @@ func (c Command) Run(ctx *Context) (err error) { return nil } - cerr := checkRequiredFlags(c.Flags, set) + cerr := checkRequiredFlags(c.Flags, context) if cerr != nil { ShowCommandHelp(context, c.Name) return cerr diff --git a/context.go b/context.go index 8af3264..3e516c8 100644 --- a/context.go +++ b/context.go @@ -309,17 +309,12 @@ func (e *errRequiredFlags) getMissingFlags() []string { return e.missingFlags } -func checkRequiredFlags(flags []Flag, set *flag.FlagSet) requiredFlagsErr { - visited := make(map[string]bool) - set.Visit(func(f *flag.Flag) { - visited[f.Name] = true - }) - +func checkRequiredFlags(flags []Flag, context *Context) requiredFlagsErr { var missingFlags []string for _, f := range flags { if rf, ok := f.(RequiredFlag); ok && rf.IsRequired() { key := strings.Split(f.GetName(), ",")[0] - if !visited[key] { + if !context.IsSet(key) { missingFlags = append(missingFlags, key) } } diff --git a/context_test.go b/context_test.go index 585ca82..f2fc250 100644 --- a/context_test.go +++ b/context_test.go @@ -495,9 +495,11 @@ func TestCheckRequiredFlags(t *testing.T) { flags.Apply(set) } set.Parse(test.parseInput) + ctx := &Context{} + context := NewContext(ctx.App, set, ctx) // logic under test - err := checkRequiredFlags(test.flags, set) + err := checkRequiredFlags(test.flags, context) // assertions if test.expectedAnError && err == nil { From f4128a02f3215e532dff0b96e21a8e2cb08389a1 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 22:54:15 -0700 Subject: [PATCH 56/58] Update command.go --- command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command.go b/command.go index e3b57db..f1ca02d 100644 --- a/command.go +++ b/command.go @@ -272,7 +272,7 @@ func reorderArgs(args []string) []string { } func translateShortOptions(set *flag.FlagSet, flagArgs Args) []string { - allCharsFlags := func(s string) bool { + allCharsFlags := func (s string) bool { for i := range s { f := set.Lookup(string(s[i])) if f == nil { From d7ec4e801357fa5ccfab53669a42f78fc1a69d39 Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 23:26:43 -0700 Subject: [PATCH 57/58] add env var tests --- context_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/context_test.go b/context_test.go index f2fc250..47313e1 100644 --- a/context_test.go +++ b/context_test.go @@ -407,6 +407,7 @@ func TestCheckRequiredFlags(t *testing.T) { tdata := []struct { testCase string parseInput []string + envVarInput [2]string flags []Flag expectedAnError bool expectedErrorContents []string @@ -435,6 +436,13 @@ func TestCheckRequiredFlags(t *testing.T) { }, parseInput: []string{"--requiredFlag", "myinput"}, }, + { + testCase: "required_and_present_via_env_var", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true, EnvVar: "REQUIRED_FLAG"}, + }, + envVarInput: [2]string{"REQUIRED_FLAG", "true"}, + }, { testCase: "required_and_optional", flags: []Flag{ @@ -452,6 +460,15 @@ func TestCheckRequiredFlags(t *testing.T) { parseInput: []string{"--optionalFlag", "myinput"}, expectedAnError: true, }, + { + testCase: "required_and_optional_and_optional_present_via_env_var", + flags: []Flag{ + StringFlag{Name: "requiredFlag", Required: true}, + StringFlag{Name: "optionalFlag", EnvVar: "OPTIONAL_FLAG"}, + }, + envVarInput: [2]string{"OPTIONAL_FLAG", "true"}, + expectedAnError: true, + }, { testCase: "required_and_optional_and_required_present", flags: []Flag{ @@ -495,8 +512,13 @@ func TestCheckRequiredFlags(t *testing.T) { flags.Apply(set) } set.Parse(test.parseInput) + if test.envVarInput[0] != "" { + os.Clearenv() + os.Setenv(test.envVarInput[0], test.envVarInput[1]) + } ctx := &Context{} context := NewContext(ctx.App, set, ctx) + context.Command.Flags = test.flags // logic under test err := checkRequiredFlags(test.flags, context) From 60fb2972328d6a7487c6821a58a86d476167c2bd Mon Sep 17 00:00:00 2001 From: Lynn Cyrin Date: Thu, 1 Aug 2019 23:27:34 -0700 Subject: [PATCH 58/58] remove help assertion stuff --- app_test.go | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/app_test.go b/app_test.go index 658ca21..ad49464 100644 --- a/app_test.go +++ b/app_test.go @@ -994,17 +994,7 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { } for _, test := range tdata { t.Run(test.testCase, func(t *testing.T) { - // setup - undo HelpPrinter mock when finished - oldPrinter := HelpPrinter - defer func() { - HelpPrinter = oldPrinter - }() - // setup - mock HelpPrinter - var helpPrinterWasCalled = false - HelpPrinter = func(w io.Writer, template string, data interface{}) { - helpPrinterWasCalled = true - } - // setup - app + // setup app := NewApp() app.Flags = test.appFlags app.Commands = test.appCommands @@ -1013,9 +1003,6 @@ func TestRequiredFlagAppRunBehavior(t *testing.T) { err := app.Run(test.appRunInput) // assertions - if helpPrinterWasCalled == false { - t.Errorf("HelpPrinter expected to be called, but was not") - } if test.expectedAnError && err == nil { t.Errorf("expected an error, but there was none") }