cobra deprecate ExactValidArgs and test combinations of args validators

This is a subset of #841, which was approved by @jharshman and reviewed by @marckhouzam .

Fix #838 and Fix #745.

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 six 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 three combinations missing, which is what was pointed out in https://github.com/spf13/cobra/issues/838 at first.

Hence, this PR enforces the orthogonality in the code by moving the validation of ValidArgs from args.go to command.go, so that:

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

The README is updated accordingly.

Mar 22 '22 18:03 umarcor

Thanks for splitting this out @umarcor. I'll have a look soon.

Mar 23 '22 10:03 marckhouzam

@marckhouzam thanks for a very interesting analysis! It seems that the "main" purpose of ValidArgs changed since it was introduced in 2015:

https://github.com/spf13/cobra/commit/9b2e6822e5819fdc8d6c80654b40ba4f805c690a (#69): ValidArgs and BashCompletionFunction were introduced, to be used in shell completion. The arguments were not validated:

// List of all valid non-flag arguments, used for bash completions *TODO* actually validate these
ValidArgs []string

https://github.com/spf13/cobra/commit/d89c499964451591c4e8142464f0d558c4a3698d (#284): the management of arguments was reworked, introducing option TakesArgs, which supported Legacy, None, Arbitrary or ValidOnly.

`Legacy` (or default) the rules are as follows:
- root commands with no subcommands can take arbitrary arguments
- root commands with subcommands will do subcommand validity checking
- subcommands will always accept arbitrary arguments and do no subsubcommand validity checking

`None` the command will be rejected if there are any left over arguments after parsing flags.

`Arbitrary` any additional values left after parsing flags will be passed in to your `Run` function.

`ValidOnly` you must define all valid (non-subcommand) arguments to your command. These are defined in a slice name ValidArgs. For example a command which only takes the argument "one" or "two" would be defined as:

https://github.com/spf13/cobra/commit/f20b4e9c32bb3e9d44773ca208db814f24dcd21b (#284): the previous commit was refactored and TakesArgs was removed and built-in validators were added:

Validation of positional arguments can be specified using the `Args` field.

The follow validators are built in:

- `NoArgs` - the command will report an error if there are any positional args.
- `ArbitraryArgs` - the command will accept any args.
- `OnlyValidArgs` - the command will report an error if there are any positional args that are not in the ValidArgs list.
- `MinimumNArgs(int)` - the command will report an error if there are not at least N positional args.
- `MaximumNArgs(int)` - the command will report an error if there are more than N positional args.
- `ExactArgs(int)` - the command will report an error if there are not exactly N positional args.
- `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.

At that point, any of MinimumNArgs, MaximumNArgs, ExactArgs or RangeArgs (which are mutually exclusive) could be combined with OnlyValidArgs, by writing a custom validation function. However, the tests did not include those use cases.

https://github.com/spf13/cobra/commit/f619abc1d7edb30544d1c956719d203ace73e9b1 (#765): ExactValidArgs was added:

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)
	}
}

So, the inconsistency was introduced in #765. MinimumNValidArgs, MaximumNValidArgs and RangeValidArgs were missing.

It was not obvious back then, that the implementation should have been a generic helper to combine validators. MatchAll was proposed some months later (#896), which provided the generic solution. ExactValidArgs could have been explicitly deprecated in #896 in favour of showcasing a generic solution, but it was overlooked (even though we did discuss about it).

Note that this PR was created in March 2019 (#841), between #765 (October 2018) and #896 (June 2019).

Note also that there was no reference to the completion feature in neither of #284, #765, #841 or #896, despite ValidArgs apparently still being used for that purpose as well. It seems that it was mostly forgotten until you started to work on it 2 years ago.

Overall, I think that enforcing the valid args in the kubectl example you explained was not a supported use case until you reworked and extended the usability of the completion feature. Nevertheless, I agree there is no need to apply the OnlyValidArgs validator implicitly, because the work introduced in the codebase since this PR was proposed makes it sensible to instead document how to do it explicitly. Therefore, I will update this PR to:

Remove https://github.com/spf13/cobra/pull/1643/files#diff-4d7d1923693fc5ce892add2ea2907a744e77ea0b50c1939ccc5067cb48a466a3R1013-R1015.
Not deprecate OnlyValidArgs, but ExactValidArgs only.
Convert all the tests added in this PR to use the explicit solution (combine OnlyValidArgs with each of the other validators, through MatchAll).
Update the User Guide to explain which validators are mutually exclusive, and which are orthogonal (can be combined).

As a result, the outcomes of this PR will be:

Deprecate but not remove ExactValidArgs.
Add tests to check the combination of built-in validators which are orthogonal.
Document how to combine the built-in validators through the built-in combination function.

Then, in a follow-up PR, we can discuss how to enhance OnlyValidArgs with ValidArgsFunction in order to support the kubectl example with aliases.

Mar 27 '22 21:03 umarcor