Repository: spf13/cobra
Pull request #896 (2019-06-28 21:52:02)

enable composing PositionalArgs
This may end up being helpful related to issues like #838 or #745... 

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/spf13/cobra?pullRequest=896) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/spf13/cobra?pullRequest=896) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/spf13/cobra?pullRequest=896) it.</sub>
umarcor: Hi @nelz9999! `ExactValidArgs` is deprecated in #841, since checks are orthogonalized: checking the number of arguments is decoupled from checking the values. Please see https://github.com/spf13/cobra/pull/841#issuecomment-474604947 and https://github.com/spf13/cobra/pull/841#issuecomment-474617017. Could you, please, have a look at #841 and propose this PR on top of it? I'll be pleased to include it, so that everything is merged together.

Apart from that, I would expect this PR to include additional tests to check the new feature. Some example would also be helpful to better show what you mean with 'composite positional arguments'.
nelz9999: Hey @umarcor... 

I took your suggestions of tests & docs, and to leave `ExactValidArgs` alone.

However, I don't think I should add on top of your #841... There's a lot of pushback and flux on that PR, which I don't think this addition needs to be a part of, since it's simply additive. I.e. this PR could get merged without any impact on #841.

umarcor: > I took your suggestions of tests & docs, and to leave `ExactValidArgs` alone.

Thanks!

> However, I don't think I should add on top of your #841... There's a lot of pushback and flux on that PR, which I don't think this addition needs to be a part of, since it's simply additive. I.e. this PR could get merged without any impact on #841.

Agree. Since `ExactValidArgs` is not modified now, both PRs are independent. Although they cannot be automatically merged (see https://github.com/umarcor/cobra/compare/feat-matchvalid...nelz9999:nelz%2Fargs?diff=split or https://github.com/nelz9999/cobra/compare/nelz%2Fargs...umarcor:feat-matchvalid?diff=split), I believe it will not be hard to do it.

Overall, LGTM. I would just suggest to reconsider the name of the new function/feature, tho. When I read 'composing PositionalArgs' I imagined handling positional arguments composed of more than a single word. IMHO, `MatchAll` (as proposed by @Antolius in [#745](https://github.com/spf13/cobra/issues/745#issuecomment-441326468)), `MatchMany`, `MatchMultiple`, `CheckArgs`, `ArgChecks`, `CombinedMatch`, etc. would better fit the usage: combine multiple existing checks (either built-in or custom); e.g. check ExactArgs length along with other qualities.
nelz9999: @umarcor I've updated the naming.
umarcor: > @umarcor I've updated the naming.

I saw that and I think that this PR is ready to merge; that's why I didn't add any further comment.

Unfortunately, maintainers with write permissions in this repo seem to be quite busy lately. For example #841 is ready since almost 4 months ago, but it has not been merged yet. A similar situation happened with #817.

Hence, we need to be patient and wait until @sp13, @eparis, @bogem, or any other can allocate some time. I don't know whether @jharshman has permissions or is just a regular contributor.
umarcor: > as an aside, looks like Travis CI is failing due to some shellcheck errors.

Indeed... https://github.com/spf13/cobra/pull/889#issuecomment-508921044
umarcor: @nelz9999, now that CI of the master branch is fixed, would you mind rebasing? This is so that it is not shown as 'failing', which can lead maintainers to think that this is not ready.
nelz9999: @umarcor Done and done!
umarcor: Request to be included in [1.0.0](https://github.com/spf13/cobra/milestone/2). Ref #959.
github-actions[bot]: This PR is being marked as stale due to a long period of inactivity
umarcor: Not stale, but ready to be merged.
umarcor: Is there anything preventing this from being merged?
veleek: Ping this issue!  Anything to prevent this from being merged?
katexochen: Will this be merged soon?
umarcor: @katexochen, I wouldn't bet on that. The criteria for merging PRs in this project is arbitrary and it is frequent that ready-to-merge PRs are ignored for years.
katexochen: @umarcor thanks for the answer, already thought so. And thanks to @nelz9999 for the work anyway.
jpmcb: Hi @nelz9999 - thanks so much for this, and thanks all for the patience. Any possibility you are around to rebase this and fix a merge conflict in the readme? If I don't hear from you in a week or so, I'll close this and hand it back over to the community.
nelz9999: @jpmcb Done!

Patch:
diff --git a/args.go b/args.go
index 70e9b26..20a022b 100644
--- a/args.go
+++ b/args.go
@@ -107,3 +107,15 @@ func RangeArgs(min int, max int) PositionalArgs {
 		return nil
 	}
 }
+
+// MatchAll allows combining several PositionalArgs to work in concert.
+func MatchAll(pargs ...PositionalArgs) PositionalArgs {
+	return func(cmd *Command, args []string) error {
+		for _, parg := range pargs {
+			if err := parg(cmd, args); err != nil {
+				return err
+			}
+		}
+		return nil
+	}
+}
diff --git a/user_guide.md b/user_guide.md
index 3ab111c..e87cdf2 100644
--- a/user_guide.md
+++ b/user_guide.md
@@ -315,6 +315,7 @@ The following validators are built in:
 - `ExactArgs(int)` - the command will report an error if there are not exactly N positional args.
 - `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:
 


Test patch:
diff --git a/args_test.go b/args_test.go
index c81b212..ded3d0d 100644
--- a/args_test.go
+++ b/args_test.go
@@ -1,30 +1,47 @@
 package cobra
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 )
 
-func TestNoArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: NoArgs, Run: emptyRun}
+func getCommand(args PositionalArgs, withValid bool) *Command {
+	c := &Command{
+		Use:  "c",
+		Args: args,
+		Run:  emptyRun,
+	}
+	if withValid {
+		c.ValidArgs = []string{"one", "two", "three"}
+	}
+	return c
+}
 
-	output, err := executeCommand(c)
+func expectSuccess(output string, err error, t *testing.T) {
 	if output != "" {
-		t.Errorf("Unexpected string: %v", output)
+		t.Errorf("Unexpected output: %v", output)
 	}
 	if err != nil {
 		t.Fatalf("Unexpected error: %v", err)
 	}
 }
 
-func TestNoArgsWithArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: NoArgs, Run: emptyRun}
-
-	_, err := executeCommand(c, "illegal")
+func validWithInvalidArgs(err error, t *testing.T) {
 	if err == nil {
 		t.Fatal("Expected an error")
 	}
+	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) {
+	if err == nil {
+		t.Fatal("Expected an error")
+	}
 	got := err.Error()
 	expected := `unknown command "illegal" for "c"`
 	if got != expected {
@@ -32,202 +49,144 @@ func TestNoArgsWithArgs(t *testing.T) {
 	}
 }
 
-func TestOnlyValidArgs(t *testing.T) {
-	c := &Command{
-		Use:       "c",
-		Args:      OnlyValidArgs,
-		ValidArgs: []string{"one", "two"},
-		Run:       emptyRun,
+func minimumNArgsWithLessArgs(err error, t *testing.T) {
+	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)
+	}
+}
 
-	output, err := executeCommand(c, "one", "two")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
+func maximumNArgsWithMoreArgs(err error, t *testing.T) {
+	if err == nil {
+		t.Fatal("Expected an error")
 	}
-	if err != nil {
-		t.Fatalf("Unexpected error: %v", err)
+	got := err.Error()
+	expected := "accepts at most 2 arg(s), received 3"
+	if got != expected {
+		t.Fatalf("Expected %q, got %q", expected, got)
 	}
 }
 
-func TestOnlyValidArgsWithInvalidArgs(t *testing.T) {
-	c := &Command{
-		Use:       "c",
-		Args:      OnlyValidArgs,
-		ValidArgs: []string{"one", "two"},
-		Run:       emptyRun,
+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)
+	}
+}
 
-	_, err := executeCommand(c, "three")
+func rangeArgsWithInvalidCount(err error, t *testing.T) {
 	if err == nil {
 		t.Fatal("Expected an error")
 	}
-
 	got := err.Error()
-	expected := `invalid argument "three" for "c"`
+	expected := "accepts between 2 and 4 arg(s), received 1"
 	if got != expected {
-		t.Errorf("Expected: %q, got: %q", expected, got)
+		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 := &Command{Use: "c", Args: ArbitraryArgs, Run: emptyRun}
+	c := getCommand(ArbitraryArgs, false)
 	output, err := executeCommand(c, "a", "b")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	expectSuccess(output, err, t)
 }
 
 func TestMinimumNArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: MinimumNArgs(2), Run: emptyRun}
+	c := getCommand(MinimumNArgs(2), false)
 	output, err := executeCommand(c, "a", "b", "c")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	expectSuccess(output, err, t)
 }
 
 func TestMinimumNArgsWithLessArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: MinimumNArgs(2), Run: emptyRun}
+	c := getCommand(MinimumNArgs(2), false)
 	_, err := executeCommand(c, "a")
-
-	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)
-	}
+	minimumNArgsWithLessArgs(err, t)
 }
 
 func TestMaximumNArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: MaximumNArgs(3), Run: emptyRun}
+	c := getCommand(MaximumNArgs(3), false)
 	output, err := executeCommand(c, "a", "b")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	expectSuccess(output, err, t)
 }
 
 func TestMaximumNArgsWithMoreArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: MaximumNArgs(2), Run: emptyRun}
+	c := getCommand(MaximumNArgs(2), false)
 	_, err := executeCommand(c, "a", "b", "c")
-
-	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)
-	}
+	maximumNArgsWithMoreArgs(err, t)
 }
 
 func TestExactArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: ExactArgs(3), Run: emptyRun}
+	c := getCommand(ExactArgs(3), false)
 	output, err := executeCommand(c, "a", "b", "c")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	expectSuccess(output, err, t)
 }
 
 func TestExactArgsWithInvalidCount(t *testing.T) {
-	c := &Command{Use: "c", Args: ExactArgs(2), Run: emptyRun}
+	c := getCommand(ExactArgs(2), false)
 	_, err := executeCommand(c, "a", "b", "c")
-
-	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)
-	}
+	exactArgsWithInvalidCount(err, t)
 }
 
 func TestExactValidArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: ExactValidArgs(3), ValidArgs: []string{"a", "b", "c"}, Run: emptyRun}
-	output, err := executeCommand(c, "a", "b", "c")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	c := getCommand(ExactValidArgs(3), true)
+	output, err := executeCommand(c, "three", "one", "two")
+	expectSuccess(output, err, t)
 }
 
 func TestExactValidArgsWithInvalidCount(t *testing.T) {
-	c := &Command{Use: "c", Args: ExactValidArgs(2), Run: emptyRun}
-	_, err := executeCommand(c, "a", "b", "c")
-
-	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)
-	}
+	c := getCommand(ExactValidArgs(2), false)
+	_, err := executeCommand(c, "three", "one", "two")
+	exactArgsWithInvalidCount(err, t)
 }
 
 func TestExactValidArgsWithInvalidArgs(t *testing.T) {
-	c := &Command{
-		Use:       "c",
-		Args:      ExactValidArgs(1),
-		ValidArgs: []string{"one", "two"},
-		Run:       emptyRun,
-	}
-
-	_, err := executeCommand(c, "three")
-	if err == nil {
-		t.Fatal("Expected an error")
-	}
-
-	got := err.Error()
-	expected := `invalid argument "three" for "c"`
-	if got != expected {
-		t.Errorf("Expected: %q, got: %q", expected, got)
-	}
+	c := getCommand(ExactValidArgs(3), true)
+	_, err := executeCommand(c, "three", "a", "two")
+	validWithInvalidArgs(err, t)
 }
 
 func TestRangeArgs(t *testing.T) {
-	c := &Command{Use: "c", Args: RangeArgs(2, 4), Run: emptyRun}
+	c := getCommand(RangeArgs(2, 4), false)
 	output, err := executeCommand(c, "a", "b", "c")
-	if output != "" {
-		t.Errorf("Unexpected output: %v", output)
-	}
-	if err != nil {
-		t.Errorf("Unexpected error: %v", err)
-	}
+	expectSuccess(output, err, t)
 }
 
 func TestRangeArgsWithInvalidCount(t *testing.T) {
-	c := &Command{Use: "c", Args: RangeArgs(2, 4), Run: emptyRun}
+	c := getCommand(RangeArgs(2, 4), false)
 	_, err := executeCommand(c, "a")
-
-	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)
-	}
+	rangeArgsWithInvalidCount(err, t)
 }
 
 func TestRootTakesNoArgs(t *testing.T) {
@@ -285,3 +244,51 @@ func TestChildTakesArgs(t *testing.T) {
 		t.Fatalf("Unexpected error: %v", err)
 	}
 }
+
+func TestMatchAll(t *testing.T) {
+	// Somewhat contrived example check that ensures there are exactly 3
+	// arguments, and each argument is exactly 2 bytes long.
+	pargs := MatchAll(
+		ExactArgs(3),
+		func(cmd *Command, args []string) error {
+			for _, arg := range args {
+				if len([]byte(arg)) != 2 {
+					return fmt.Errorf("expected to be exactly 2 bytes long")
+				}
+			}
+			return nil
+		},
+	)
+
+	testCases := map[string]struct {
+		args []string
+		fail bool
+	}{
+		"happy path": {
+			[]string{"aa", "bb", "cc"},
+			false,
+		},
+		"incorrect number of args": {
+			[]string{"aa", "bb", "cc", "dd"},
+			true,
+		},
+		"incorrect number of bytes in one arg": {
+			[]string{"aa", "bb", "abc"},
+			true,
+		},
+	}
+
+	rootCmd := &Command{Use: "root", Args: pargs, Run: emptyRun}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			_, err := executeCommand(rootCmd, tc.args...)
+			if err != nil && !tc.fail {
+				t.Errorf("unexpected: %v\n", err)
+			}
+			if err == nil && tc.fail {
+				t.Errorf("expected error")
+			}
+		})
+	}
+}
diff --git a/cobra/cmd/golden_test.go b/cobra/cmd/golden_test.go
index 99b92e3..832ea53 100644
--- a/cobra/cmd/golden_test.go
+++ b/cobra/cmd/golden_test.go
@@ -43,7 +43,7 @@ func compareFiles(pathA, pathB string) error {
 			// Don't execute diff if it can't be found.
 			return nil
 		}
-		diffCmd := exec.Command(diffPath, "-u", pathA, pathB)
+		diffCmd := exec.Command(diffPath, "-u", "--strip-trailing-cr", pathA, pathB)
 		diffCmd.Stdout = output
 		diffCmd.Stderr = output