args_test refactorization after generalizing ValidArgs

[umarcor]

Fix #838 and Fix #745.

5 of the commits in this PR are about refactoring args_test.go. The sixth one (feat: generalize ValidArgs; use it implicitly with any validator) moves the validation of ValidArgs from args.go to command.go. As a result:

• Any validator can be used along with ValidArgs. ValidArgs is checked first, and then the defined validator.
• OnlyValidArgs and ExactValidArgs are deprecated.

args_test.go is updated accordingly:

=== RUN   TestNoArgs
--- PASS: TestNoArgs (0.00s)
=== RUN   TestNoArgsWithArgs
--- PASS: TestNoArgsWithArgs (0.00s)
=== RUN   TestNoArgsWithArgsWithValid
--- PASS: TestNoArgsWithArgsWithValid (0.00s)
=== RUN   TestArbitraryArgs
--- PASS: TestArbitraryArgs (0.00s)
=== RUN   TestArbitraryArgsWithValid
--- PASS: TestArbitraryArgsWithValid (0.00s)
=== RUN   TestArbitraryArgsWithValidWithInvalidArgs
--- PASS: TestArbitraryArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestMinimumNArgs
--- PASS: TestMinimumNArgs (0.00s)
=== RUN   TestMinimumNArgsWithValid
--- PASS: TestMinimumNArgsWithValid (0.00s)
=== RUN   TestMinimumNArgsWithValidWithInvalidArgs
--- PASS: TestMinimumNArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestMinimumNArgsWithLessArgs
--- PASS: TestMinimumNArgsWithLessArgs (0.00s)
=== RUN   TestMinimumNArgsWithLessArgsWithValid
--- PASS: TestMinimumNArgsWithLessArgsWithValid (0.00s)
=== RUN   TestMinimumNArgsWithLessArgsWithValidWithInvalidArgs
--- PASS: TestMinimumNArgsWithLessArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestMaximumNArgs
--- PASS: TestMaximumNArgs (0.00s)
=== RUN   TestMaximumNArgsWithValid
--- PASS: TestMaximumNArgsWithValid (0.00s)
=== RUN   TestMaximumNArgsWithValidWithInvalidArgs
--- PASS: TestMaximumNArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestMaximumNArgsWithMoreArgs
--- PASS: TestMaximumNArgsWithMoreArgs (0.00s)
=== RUN   TestMaximumNArgsWithMoreArgsWithValid
--- PASS: TestMaximumNArgsWithMoreArgsWithValid (0.00s)
=== RUN   TestMaximumNArgsWithMoreArgsWithValidWithInvalidArgs
--- PASS: TestMaximumNArgsWithMoreArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestExactArgs
--- PASS: TestExactArgs (0.00s)
=== RUN   TestExactArgsWithValid
--- PASS: TestExactArgsWithValid (0.00s)
=== RUN   TestExactArgsWithValidWithInvalidArgs
--- PASS: TestExactArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestExactArgsWithInvalidCount
--- PASS: TestExactArgsWithInvalidCount (0.00s)
=== RUN   TestExactArgsWithInvalidCountWithValid
--- PASS: TestExactArgsWithInvalidCountWithValid (0.00s)
=== RUN   TestExactArgsWithInvalidCountWithValidWithInvalidArgs
--- PASS: TestExactArgsWithInvalidCountWithValidWithInvalidArgs (0.00s)
=== RUN   TestRangeArgs
--- PASS: TestRangeArgs (0.00s)
=== RUN   TestRangeArgsWithValid
--- PASS: TestRangeArgsWithValid (0.00s)
=== RUN   TestRangeArgsWithValidWithInvalidArgs
--- PASS: TestRangeArgsWithValidWithInvalidArgs (0.00s)
=== RUN   TestRangeArgsWithInvalidCount
--- PASS: TestRangeArgsWithInvalidCount (0.00s)
=== RUN   TestRangeArgsWithInvalidCountWithValid
--- PASS: TestRangeArgsWithInvalidCountWithValid (0.00s)
=== RUN   TestRangeArgsWithInvalidCountWithValidWithInvalidArgs
--- PASS: TestRangeArgsWithInvalidCountWithValidWithInvalidArgs (0.00s)
=== RUN   TestRootTakesNoArgs
--- PASS: TestRootTakesNoArgs (0.00s)
=== RUN   TestRootTakesArgs
--- PASS: TestRootTakesArgs (0.00s)
=== RUN   TestChildTakesNoArgs
--- PASS: TestChildTakesNoArgs (0.00s)
=== RUN   TestChildTakesArgs
--- PASS: TestChildTakesArgs (0.00s)

[CLAassistant]

All committers have signed the CLA.

[jharshman]

IMO this PR overreaches a little over what was originally intended. Changing to Go modules is great but not required for this feature.

I think there is a case for improvement here but perhaps it can be approached differently. Issue #838 mentions that there is not currently a method of combining these validation functions. A fix for this could to introduce a method of chaining them.

[umarcor]

IMO this PR overreaches a little over what was originally intended. Changing to Go modules is great but not required for this feature.

This PR was based on #840 because I expected that branch to be merged before this, so it could be fast-forwarded. #840 provides some interesting features, such as fixing CI, which would help evaluate this PR.

Anyway, I have now made this PR independent from #840.

I think there is a case for improvement here but perhaps it can be approached differently. Issue #838 mentions that there is not currently a method of combining these validation functions. A fix for this could to introduce a method of chaining them.

I think that it does not make sense to combine any of NoArgs, ArbitraryArgs, MinimumNArgs, MaximumNArgs, ExactArgs, RangeArgs, because all of them are mutually exclusive. Instead, my proposal is to apply ValidArgs (when defined) to the selected validator. That's why OnlyValidArgs and ExactValidArgs are deprecated.

Moreover, if a user wants to combine any of the provided validators and extend it with custom logic, an example can be added.

[jharshman]

Thanks for making this independent of #840 .

Maybe some clarification on the goal here was needed because it seems to have strayed away from the original issue you referenced. That issue proposed the addition of validation functions which were combinations of what is currently available. Hence why I suggested adding a method to allow the chaining of validation functions.

However, I think I see where you're coming from. You want to remove OnlyValidArgs and ExactValidArgs and instead have a check within the validation function to decide whether or not the argument slice matched some provided valid arguments.

However, this check inside the validation function contains the same logic as OnlyValidArgs. You could perhaps keep OnlyValidArgs and therefore only have to depreciate one function instead of two.

[umarcor]

@jharshman, I had the same point of view as you when I started to implement this. As I went forward and I saw the codebase, I concluded that the proposed solution is the best. Some hints to better understand the changes:

• This 7 lines are moved from https://github.com/spf13/cobra/blob/master/args.go#L36-L42 to https://github.com/spf13/cobra/pull/841/files#diff-9c42b524cb14ca001c61267826cbefb1.
• As a result, OnlyValidArgs and ExactValidArgs are not required anymore (https://github.com/spf13/cobra/pull/841/files#diff-07ee57a49d1c0b692bd79676cc31dd6f) because ArbitraryArgs and ExactArgs do now check ValidArgs.
• Moreover, ValidArgs is now checked with no regard to the chosen validator.
• This is explained in the README: https://github.com/spf13/cobra/pull/841/files#diff-04c6e90faac2675aa89e2176d2eec7d8
• All the changes in arg_test.go are to validate the points above: https://github.com/spf13/cobra/pull/841/files#diff-748c41438b4135ae7035c2af1228e349 (see also https://travis-ci.org/spf13/cobra/jobs/508608833#L543-L602)

Maybe some clarification on the goal here was needed because it seems to have strayed away from the original issue you referenced. That issue proposed the addition of validation functions which were combinations of what is currently available. Hence why I suggested adding a method to allow the chaining of validation functions.

The issue requested some method to combine e.g. MinimumNArgs(1) with OnlyValidArgs. That's possible with the current proposal:

Args: cobra.MinimumNArgs(1),
ValidArgs: []string{"one", "two", "three"},

compared to the prototype suggested in #838:

Args: cobra.MatchValid(cobra.MinimumNArgs(1)),
ValidArgs: []string{"one", "two", "three"},

Moreover, in #838 it was suggested that it would be better not to require the user to type MatchValid:

Certainly, MatchValid can be used implicitly when ValidArgs is defined, so the user is not required to type it. Therefore, OnlyValidArgs and ExactValidArgs can be deprecated from the public API.

---

However, I think I see where you're coming from. You want to remove OnlyValidArgs and ExactValidArgs and instead have a check within the validation function to decide whether or not the argument slice matched some provided valid arguments.

All of the original validators care about the number of arguments that a command accepts: NoArgs, ArbitraryArgs, MinimumNArgs(int), MaximumNArgs(int), ExactArgs(int), RangeArgs(min, max). We can consider it an enumeration of 6 items.

When ValidArgs was introduced, up to 6 additional validators might have been added, in order to provide functions covering the full combination matrix. Certainly, ValidArgs is a boolean condition which is orthogonal to Args, because it is focused on the content of the args, not the number of them. Therefore, the full matrix covers 6 x 2 = 12 combinations. However, NoArgs + ValidArgs == NoArgs + !ValidArgs, so 11.

Nonetheless, the implementation was partial and a single one was introduced. Later, a second one was added. Still, there are 3 combinations missing, which is what was pointed out in #838 at first.

Hence, the design decission is to provide one explicit validator for each combination of the multiple criterias to filter the arguments (original approach), or to enforce the orthogonality in the code (proposed approach).

However, this check inside the validation function contains the same logic as OnlyValidArgs. You could perhaps keep OnlyValidArgs and therefore only have to depreciate one function instead of two.

It is possible to make I now made OnlyValidArgs an alias of ArbitraryArgs and ExactArgs an alias of ExactValidArgs, so that backward compatibility is retained. I don't think it makes sense to deprecate only one of them (see comment above).

[jharshman]

@umarcor thanks for taking the time to detail your approach and solution. Looks like one of the tests doesn't like the go tool vet command.

+vet: invoking "go tool vet" directly is unsupported; use "go vet"

[umarcor]

@umarcor thanks for taking the time to detail your approach and solution. Looks like one of the tests doesn't like the go tool vet command.

+vet: invoking "go tool vet" directly is unsupported; use "go vet"

This is why I originally based this PR on top of #840. Those CI issues are fixed there, as commented above. The commit that fixes it is precisely: bbec970. Nonetheless, I think that it would be better to use golangci-lint. That's what I'm doing in my fork (umarcor@3b60284), while I wait for maintainers to respond.

[umarcor]

#840 is merged now, so I rebased this on top of master. Since all the tests are passing, I believe it is ready for review, @eparis.

[jharshman]

LGTM

[jharshman]

Looks fine to me. @eparis

[umarcor]

ping

[umarcor]

Can we change the milestone of this PR to 1.5.0?

[johnSchnake]

This has an approval and green tests; is it waiting for anything else? I know this PR is connected to a lot of issues and if everyones happy I'm just curious what the hold is right now? [FWIW I haven't reviewed this, just cleaning up the backlog and kept finding links to this PR]

[umarcor]

@johnSchnake this is only waiting for some maintainer to decide merging it. It was approved by a maintainer several years ago, and then reviewed by a user who is a maintainer now. It was not merged since then because none of the currently active maintainers had time to sit down, read it, and understand what's it about. That's why I split #1426 in november, and I now split #1643. Please, review #1643, since that's the commit that modifies the functionality. This PR is now just style/refactorization based on #1643.

BTW, you might want to merge #1612, which is a low-hanging fruit after #1547 was merged last week.

[github-actions]

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.