Merge a9dbe74c87 into 2e8ba6f308

args.go

@@ -7,6 +7,25 @@ import (
 
 type PositionalArgs func(cmd *Command, args []string) error
 
+// validateArgs returns an error if there are any positional args that are not in
+// the `ValidArgs` field of `Command`
+func validateArgs(cmd *Command, args []string) error {
+	if len(cmd.ValidArgs) > 0 {
+		// Remove any description that may be included in ValidArgs.
+		// A description is following a tab character.
+		var validArgs []string
+		for _, v := range cmd.ValidArgs {
+			validArgs = append(validArgs, strings.Split(v, "\t")[0])
+		}
+		for _, v := range args {
+			if !stringInSlice(v, validArgs) {
+				return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.findSuggestions(args[0]))
+			}
+		}
+	}
+	return nil
+}
+
 // Legacy arg validation has the following behaviour:
 // - root commands with no subcommands can take arbitrary arguments
 // - root commands with subcommands will do subcommand validity checking
@@ -32,25 +51,6 @@ func NoArgs(cmd *Command, args []string) error {
 	return nil
 }
 
-// OnlyValidArgs returns an error if any args are not in the list of ValidArgs.
-func OnlyValidArgs(cmd *Command, args []string) error {
-	if len(cmd.ValidArgs) > 0 {
-		// Remove any description that may be included in ValidArgs.
-		// A description is following a tab character.
-		var validArgs []string
-		for _, v := range cmd.ValidArgs {
-			validArgs = append(validArgs, strings.Split(v, "\t")[0])
-		}
-
-		for _, v := range args {
-			if !stringInSlice(v, validArgs) {
-				return fmt.Errorf("invalid argument %q for %q%s", v, cmd.CommandPath(), cmd.findSuggestions(args[0]))
-			}
-		}
-	}
-	return nil
-}
-
 // ArbitraryArgs never returns an error.
 func ArbitraryArgs(cmd *Command, args []string) error {
 	return nil
@@ -86,18 +86,6 @@ func ExactArgs(n int) PositionalArgs {
 	}
 }
 
-// ExactValidArgs returns an error if
-// there are not exactly N positional args OR
-// there are any positional args that are not in the `ValidArgs` field of `Command`
-func ExactValidArgs(n int) PositionalArgs {
-	return func(cmd *Command, args []string) error {
-		if err := ExactArgs(n)(cmd, args); err != nil {
-			return err
-		}
-		return OnlyValidArgs(cmd, args)
-	}
-}
-
 // RangeArgs returns an error if the number of args is not within the expected range.
 func RangeArgs(min int, max int) PositionalArgs {
 	return func(cmd *Command, args []string) error {
@@ -119,3 +107,18 @@ func MatchAll(pargs ...PositionalArgs) PositionalArgs {
 		return nil
 	}
 }
+
+// ExactValidArgs returns an error if there are not exactly N positional args OR
+// there are any positional args that are not in the `ValidArgs` field of `Command`
+//
+// Deprecated: now `ExactArgs` honors `ValidArgs`, when defined and not empty
+func ExactValidArgs(n int) PositionalArgs {
+	return ExactArgs(n)
+}
+
+// OnlyValidArgs returns an error if any args are not in the list of `ValidArgs`.
+//
+// Deprecated: now `ArbitraryArgs` honors `ValidArgs`, when defined and not empty
+func OnlyValidArgs(cmd *Command, args []string) error {
+	return ArbitraryArgs(cmd, args)
+}

args_test.go

@@ -6,188 +6,136 @@ import (
 	"testing"
 )
 
-func getCommand(args PositionalArgs, withValid bool) *Command {
+type argsTestcase struct {
+	exerr  string         // Expected error key (see map[string][string])
+	args   PositionalArgs // Args validator
+	wValid bool           // Define `ValidArgs` in the command
+	rargs  []string       // Runtime args
+}
+
+var errStrings = map[string]string{
+	"invalid":    `invalid argument "a" for "c"`,
+	"unknown":    `unknown command "one" for "c"`,
+	"less":       "requires at least 2 arg(s), only received 1",
+	"more":       "accepts at most 2 arg(s), received 3",
+	"notexact":   "accepts 2 arg(s), received 3",
+	"notinrange": "accepts between 2 and 4 arg(s), received 1",
+}
+
+func (tc *argsTestcase) test(t *testing.T) {
 	c := &Command{
 		Use:  "c",
-		Args: args,
+		Args: tc.args,
 		Run:  emptyRun,
 	}
-	if withValid {
+	if tc.wValid {
 		c.ValidArgs = []string{"one", "two", "three"}
 	}
-	return c
-}
 
-func expectSuccess(output string, err error, t *testing.T) {
+	o, e := executeCommand(c, tc.rargs...)
-	if output != "" {
+
-		t.Errorf("Unexpected output: %v", output)
+	if len(tc.exerr) > 0 {
-	}
+		// Expect error
-	if err != nil {
+		if e == nil {
-		t.Fatalf("Unexpected error: %v", err)
+			t.Fatal("Expected an error")
+		}
+		expected, ok := errStrings[tc.exerr]
+		if !ok {
+			t.Errorf(`key "%s" is not found in map "errStrings"`, tc.exerr)
+			return
+		}
+		if got := e.Error(); got != expected {
+			t.Errorf("Expected: %q, got: %q", expected, got)
+		}
+	} else {
+		// Expect success
+		if o != "" {
+			t.Errorf("Unexpected output: %v", o)
+		}
+		if e != nil {
+			t.Fatalf("Unexpected error: %v", e)
+		}
 	}
 }
 
-func validWithInvalidArgs(err error, t *testing.T) {
+func testArgs(t *testing.T, tests map[string]argsTestcase) {
-	if err == nil {
+	for name, tc := range tests {
-		t.Fatal("Expected an error")
+		t.Run(name, tc.test)
-	}
-	got := err.Error()
-	expected := `invalid argument "a" for "c"`
-	if got != expected {
-		t.Errorf("Expected: %q, got: %q", expected, got)
 	}
 }
 
-func noArgsWithArgs(err error, t *testing.T) {
+func TestArgs_No(t *testing.T) {
-	if err == nil {
+	testArgs(t, map[string]argsTestcase{
-		t.Fatal("Expected an error")
+		"      | ":      {"", NoArgs, false, []string{}},
-	}
+		"      | Arb":   {"unknown", NoArgs, false, []string{"one"}},
-	got := err.Error()
+		"Valid | Valid": {"unknown", NoArgs, true, []string{"one"}},
-	expected := `unknown command "illegal" for "c"`
+	})
-	if got != expected {
+}
-		t.Errorf("Expected: %q, got: %q", expected, got)
+func TestArgs_Nil(t *testing.T) {
-	}
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":     {"", nil, false, []string{"a", "b"}},
+		"Valid | Valid":   {"", nil, true, []string{"one", "two"}},
+		"Valid | Invalid": {"invalid", nil, true, []string{"a"}},
+	})
+}
+func TestArgs_Arbitrary(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":     {"", ArbitraryArgs, false, []string{"a", "b"}},
+		"Valid | Valid":   {"", ArbitraryArgs, true, []string{"one", "two"}},
+		"Valid | Invalid": {"invalid", ArbitraryArgs, true, []string{"a"}},
+	})
+}
+func TestArgs_MinimumN(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":         {"", MinimumNArgs(2), false, []string{"a", "b", "c"}},
+		"Valid | Valid":       {"", MinimumNArgs(2), true, []string{"one", "three"}},
+		"Valid | Invalid":     {"invalid", MinimumNArgs(2), true, []string{"a", "b"}},
+		"      | Less":        {"less", MinimumNArgs(2), false, []string{"a"}},
+		"Valid | Less":        {"less", MinimumNArgs(2), true, []string{"one"}},
+		"Valid | LessInvalid": {"invalid", MinimumNArgs(2), true, []string{"a"}},
+	})
+}
+func TestArgs_MaximumN(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":         {"", MaximumNArgs(3), false, []string{"a", "b"}},
+		"Valid | Valid":       {"", MaximumNArgs(2), true, []string{"one", "three"}},
+		"Valid | Invalid":     {"invalid", MaximumNArgs(2), true, []string{"a", "b"}},
+		"      | More":        {"more", MaximumNArgs(2), false, []string{"a", "b", "c"}},
+		"Valid | More":        {"more", MaximumNArgs(2), true, []string{"one", "three", "two"}},
+		"Valid | MoreInvalid": {"invalid", MaximumNArgs(2), true, []string{"a", "b", "c"}},
+	})
+}
+func TestArgs_Exact(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":                 {"", ExactArgs(3), false, []string{"a", "b", "c"}},
+		"Valid | Valid":               {"", ExactArgs(3), true, []string{"three", "one", "two"}},
+		"Valid | Invalid":             {"invalid", ExactArgs(3), true, []string{"three", "a", "two"}},
+		"      | InvalidCount":        {"notexact", ExactArgs(2), false, []string{"a", "b", "c"}},
+		"Valid | InvalidCount":        {"notexact", ExactArgs(2), true, []string{"three", "one", "two"}},
+		"Valid | InvalidCountInvalid": {"invalid", ExactArgs(2), true, []string{"three", "a", "two"}},
+	})
+}
+func TestArgs_Range(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"      | Arb":                 {"", RangeArgs(2, 4), false, []string{"a", "b", "c"}},
+		"Valid | Valid":               {"", RangeArgs(2, 4), true, []string{"three", "one", "two"}},
+		"Valid | Invalid":             {"invalid", RangeArgs(2, 4), true, []string{"three", "a", "two"}},
+		"      | InvalidCount":        {"notinrange", RangeArgs(2, 4), false, []string{"a"}},
+		"Valid | InvalidCount":        {"notinrange", RangeArgs(2, 4), true, []string{"two"}},
+		"Valid | InvalidCountInvalid": {"invalid", RangeArgs(2, 4), true, []string{"a"}},
+	})
+}
+func TestArgs_DEPRECATED(t *testing.T) {
+	testArgs(t, map[string]argsTestcase{
+		"OnlyValid  | Valid | Valid":        {"", OnlyValidArgs, true, []string{"one", "two"}},
+		"OnlyValid  | Valid | Invalid":      {"invalid", OnlyValidArgs, true, []string{"a"}},
+		"ExactValid | Valid | Valid":        {"", ExactValidArgs(3), true, []string{"two", "three", "one"}},
+		"ExactValid | Valid | InvalidCount": {"notexact", ExactValidArgs(2), true, []string{"two", "three", "one"}},
+		"ExactValid | Valid | Invalid":      {"invalid", ExactValidArgs(2), true, []string{"two", "a"}},
+	})
 }
 
-func minimumNArgsWithLessArgs(err error, t *testing.T) {
+// Takes(No)Args
-	if err == nil {
-		t.Fatal("Expected an error")
-	}
-	got := err.Error()
-	expected := "requires at least 2 arg(s), only received 1"
-	if got != expected {
-		t.Fatalf("Expected %q, got %q", expected, got)
-	}
-}
-
-func maximumNArgsWithMoreArgs(err error, t *testing.T) {
-	if err == nil {
-		t.Fatal("Expected an error")
-	}
-	got := err.Error()
-	expected := "accepts at most 2 arg(s), received 3"
-	if got != expected {
-		t.Fatalf("Expected %q, got %q", expected, got)
-	}
-}
-
-func exactArgsWithInvalidCount(err error, t *testing.T) {
-	if err == nil {
-		t.Fatal("Expected an error")
-	}
-	got := err.Error()
-	expected := "accepts 2 arg(s), received 3"
-	if got != expected {
-		t.Fatalf("Expected %q, got %q", expected, got)
-	}
-}
-
-func rangeArgsWithInvalidCount(err error, t *testing.T) {
-	if err == nil {
-		t.Fatal("Expected an error")
-	}
-	got := err.Error()
-	expected := "accepts between 2 and 4 arg(s), received 1"
-	if got != expected {
-		t.Fatalf("Expected %q, got %q", expected, got)
-	}
-}
-
-func TestNoArgs(t *testing.T) {
-	c := getCommand(NoArgs, false)
-	output, err := executeCommand(c)
-	expectSuccess(output, err, t)
-}
-
-func TestNoArgsWithArgs(t *testing.T) {
-	c := getCommand(NoArgs, false)
-	_, err := executeCommand(c, "illegal")
-	noArgsWithArgs(err, t)
-}
-
-func TestOnlyValidArgs(t *testing.T) {
-	c := getCommand(OnlyValidArgs, true)
-	output, err := executeCommand(c, "one", "two")
-	expectSuccess(output, err, t)
-}
-
-func TestOnlyValidArgsWithInvalidArgs(t *testing.T) {
-	c := getCommand(OnlyValidArgs, true)
-	_, err := executeCommand(c, "a")
-	validWithInvalidArgs(err, t)
-}
-
-func TestArbitraryArgs(t *testing.T) {
-	c := getCommand(ArbitraryArgs, false)
-	output, err := executeCommand(c, "a", "b")
-	expectSuccess(output, err, t)
-}
-
-func TestMinimumNArgs(t *testing.T) {
-	c := getCommand(MinimumNArgs(2), false)
-	output, err := executeCommand(c, "a", "b", "c")
-	expectSuccess(output, err, t)
-}
-
-func TestMinimumNArgsWithLessArgs(t *testing.T) {
-	c := getCommand(MinimumNArgs(2), false)
-	_, err := executeCommand(c, "a")
-	minimumNArgsWithLessArgs(err, t)
-}
-
-func TestMaximumNArgs(t *testing.T) {
-	c := getCommand(MaximumNArgs(3), false)
-	output, err := executeCommand(c, "a", "b")
-	expectSuccess(output, err, t)
-}
-
-func TestMaximumNArgsWithMoreArgs(t *testing.T) {
-	c := getCommand(MaximumNArgs(2), false)
-	_, err := executeCommand(c, "a", "b", "c")
-	maximumNArgsWithMoreArgs(err, t)
-}
-
-func TestExactArgs(t *testing.T) {
-	c := getCommand(ExactArgs(3), false)
-	output, err := executeCommand(c, "a", "b", "c")
-	expectSuccess(output, err, t)
-}
-
-func TestExactArgsWithInvalidCount(t *testing.T) {
-	c := getCommand(ExactArgs(2), false)
-	_, err := executeCommand(c, "a", "b", "c")
-	exactArgsWithInvalidCount(err, t)
-}
-
-func TestExactValidArgs(t *testing.T) {
-	c := getCommand(ExactValidArgs(3), true)
-	output, err := executeCommand(c, "three", "one", "two")
-	expectSuccess(output, err, t)
-}
-
-func TestExactValidArgsWithInvalidCount(t *testing.T) {
-	c := getCommand(ExactValidArgs(2), false)
-	_, err := executeCommand(c, "three", "one", "two")
-	exactArgsWithInvalidCount(err, t)
-}
-
-func TestExactValidArgsWithInvalidArgs(t *testing.T) {
-	c := getCommand(ExactValidArgs(3), true)
-	_, err := executeCommand(c, "three", "a", "two")
-	validWithInvalidArgs(err, t)
-}
-
-func TestRangeArgs(t *testing.T) {
-	c := getCommand(RangeArgs(2, 4), false)
-	output, err := executeCommand(c, "a", "b", "c")
-	expectSuccess(output, err, t)
-}
-
-func TestRangeArgsWithInvalidCount(t *testing.T) {
-	c := getCommand(RangeArgs(2, 4), false)
-	_, err := executeCommand(c, "a")
-	rangeArgsWithInvalidCount(err, t)
-}
 
 func TestRootTakesNoArgs(t *testing.T) {
 	rootCmd := &Command{Use: "root", Run: emptyRun}

bash_completions_test.go

@@ -139,7 +139,7 @@ func TestBashCompletions(t *testing.T) {
 	timesCmd := &Command{
 		Use:        "times [# times] [string to echo]",
 		SuggestFor: []string{"counts"},
-		Args:       OnlyValidArgs,
+		Args:       ArbitraryArgs,
 		ValidArgs:  []string{"one", "two", "three", "four"},
 		Short:      "Echo anything to the screen more times",
 		Long:       "a slightly useless command for testing.",

command.go

@@ -1011,7 +1011,13 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {
 	return cmd, err
 }
 
+// ValidateArgs returns an error if any positional args are not in the
+// `ValidArgs` field of `Command`. Then, run the `Args` validator, if
+// specified.
 func (c *Command) ValidateArgs(args []string) error {
+	if err := validateArgs(c, args); err != nil {
+		return err
+	}
 	if c.Args == nil {
 		return ArbitraryArgs(c, args)
 	}

user_guide.md

@@ -302,15 +302,15 @@ rootCmd.MarkPersistentFlagRequired("region")
 
 ### Flag Groups
 
-If you have different flags that must be provided together (e.g. if they provide the `--username` flag they MUST provide the `--password` flag as well) then 
+If you have different flags that must be provided together (e.g. if they provide the `--username` flag they MUST provide the `--password` flag as well) then
 Cobra can enforce that requirement:
 ```go
 rootCmd.Flags().StringVarP(&u, "username", "u", "", "Username (required if password is set)")
 rootCmd.Flags().StringVarP(&pw, "password", "p", "", "Password (required if username is set)")
 rootCmd.MarkFlagsRequiredTogether("username", "password")
-``` 
+```
 
-You can also prevent different flags from being provided together if they represent mutually 
+You can also prevent different flags from being provided together if they represent mutually
 exclusive options such as specifying an output format as either `--json` or `--yaml` but never both:
 ```go
 rootCmd.Flags().BoolVar(&u, "json", false, "Output in JSON")
@@ -327,29 +327,37 @@ In both of these cases:
 ## Positional and Custom Arguments
 
 Validation of positional arguments can be specified using the `Args` field of `Command`.
-If `Args` is undefined or `nil`, it defaults to `ArbitraryArgs`.
-
 The following validators are built in:
 
-- `NoArgs` - the command will report an error if there are any positional args.
+- `NoArgs` - report an error if there are any positional args.
-- `ArbitraryArgs` - the command will accept any args.
+- `ArbitraryArgs` - accept any number of args.
-- `OnlyValidArgs` - the command will report an error if there are any positional args that are not in the `ValidArgs` field of `Command`.
+- `MinimumNArgs(int)` - report an error if less than N positional args are provided.
-- `MinimumNArgs(int)` - the command will report an error if there are not at least N positional args.
+- `MaximumNArgs(int)` - report an error if more than N positional args are provided.
-- `MaximumNArgs(int)` - the command will report an error if there are more than N positional args.
+- `ExactArgs(int)` - report an error if there are not exactly N positional args.
-- `ExactArgs(int)` - the command will report an error if there are not exactly N positional args.
+- `RangeArgs(min, max)` - report an error if the number of args is not between `min` and `max`.
-- `ExactValidArgs(int)` - the command will report an error if there are not exactly N positional args OR if there are any positional args that are not in the `ValidArgs` field of `Command`
-- `RangeArgs(min, max)` - the command will report an error if the number of args is not between the minimum and maximum number of expected args.
 - `MatchAll(pargs ...PositionalArgs)` - enables combining existing checks with arbitrary other checks (e.g. you want to check the ExactArgs length along with other qualities).
 
-An example of setting the custom validator:
+If `Args` is undefined or `nil`, it defaults to `ArbitraryArgs`.
+
+Field `ValidArgs` of type `[]string` can be defined in `Command`, in order to report an error if there are any
+positional args that are not in the list.
+This validation is executed implicitly before the validator defined in `Args`.
+
+> NOTE: `OnlyValidArgs` and `ExactValidArgs(int)` are now deprecated.
+> `ArbitraryArgs` and `ExactArgs(int)` provide the same functionality now.
+
+Moreover, it is possible to set any custom validator that satisfies `func(cmd *cobra.Command, args []string) error`.
+For example:
 
 ```go
 var cmd = &cobra.Command{
   Short: "hello",
   Args: func(cmd *cobra.Command, args []string) error {
-    if len(args) < 1 {
+    // Optionally run one of the validators provided by cobra
-      return errors.New("requires a color argument")
+    if err := cobra.MinimumNArgs(1)(cmd, args); err != nil {
+        return err
     }
+    // Run the custom validation logic
     if myapp.IsValidColor(args[0]) {
       return nil
     }