Required flags

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)
@@ -23,8 +20,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
@@ -233,9 +228,8 @@ func (a *App) Run(arguments []string) (err error) {
 		return nil
 	}
 
-	cerr := checkRequiredFlags(a.Flags, set)
+	cerr := checkRequiredFlags(a.Flags, context)
 	if cerr != nil {
-		showFlagError(a.Writer, cerr)
 		ShowAppHelp(context)
 		return cerr
 	}
@@ -364,9 +358,8 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) {
 		}
 	}
 
-	cerr := checkRequiredFlags(a.Flags, set)
+	cerr := checkRequiredFlags(a.Flags, context)
 	if cerr != nil {
-		showFlagError(a.Writer, cerr)
 		ShowSubcommandHelp(context)
 		return cerr
 	}

app_test.go

@@ -880,76 +880,134 @@ func TestAppNoHelpFlag(t *testing.T) {
 func TestRequiredFlagAppRunBehavior(t *testing.T) {
 	tdata := []struct {
 		testCase        string
-		flags           []Flag
+		appFlags        []Flag
 		appRunInput     []string
+		appCommands     []Command
 		expectedAnError bool
 	}{
+		// assertion: empty input, when a required flag is present, errors
 		{
-			// 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",
-			appRunInput:     []string{"command"},
-			flags:           []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			testCase:        "error_case_empty_input_with_required_flag_on_app",
+			appRunInput:     []string{"myCLI"},
+			appFlags:        []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, does not error
-			testCase:    "help_input_with_required_flag",
-			appRunInput: []string{"command", "--help"},
-			flags:       []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			testCase:    "error_case_empty_input_with_required_flag_on_command",
+			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_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+				}},
+			}},
+			expectedAnError: true,
+		},
+		// 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"},
+			appFlags:    []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+		},
+		{
+			testCase:    "valid_case_help_input_with_required_flag_on_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_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--help"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+				}},
+			}},
+		},
+		// 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"},
+			appFlags:        []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}},
+			expectedAnError: 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"}},
+			testCase:    "error_case_optional_input_with_required_flag_on_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_on_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,
+		},
+		// 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"},
+			appFlags:    []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+		},
+		{
+			testCase:    "valid_case_required_flag_input_on_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_on_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) {
-			// 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() {
-				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.flags
+			app.Flags = test.appFlags
+			app.Commands = test.appCommands
 
 			// logic under test
 			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")
 			}
-			if test.expectedAnError && showFlagErrorWasCalled == false {
-				t.Errorf("showFlagError expected to be called, but was not")
+			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)

command.go

@@ -135,9 +135,8 @@ func (c Command) Run(ctx *Context) (err error) {
 		return nil
 	}
 
-	cerr := checkRequiredFlags(c.Flags, set)
+	cerr := checkRequiredFlags(c.Flags, context)
 	if cerr != nil {
-		showFlagError(context.App.Writer, cerr)
 		ShowCommandHelp(context, c.Name)
 		return cerr
 	}

context.go

@@ -287,29 +287,41 @@ 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
-	})
+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, 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] {
-				missingFlags = append(missingFlags, f.GetName())
+			if !context.IsSet(key) {
+				missingFlags = append(missingFlags, key)
 			}
 		}
 	}
 
-	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

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,9 +512,16 @@ 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, set)
+			err := checkRequiredFlags(test.flags, context)
 
 			// assertions
 			if test.expectedAnError && err == nil {