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 https://github.com/spf13/cobra/issues/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: https://github.com/spf13/cobra/pull/840/commits/bbec970304026225a1f5fab1583b2c09d2bd9f0f. Nonetheless, I think that it would be better to use golangci-lint. That's what I'm doing in my fork (https://github.com/umarcor/cobra/commit/3b6028420526f3f1efad98a589e59ca3a00b943d), 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.

[umarcor]

ping

[umarcor]

ping

[umarcor]

@jharshman, is it possible to have this merged or shall we wait until some other maintainer reviews it?

[umarcor]

ping

[umarcor]

[umarcor]

@hilary, I saw that you merged this PR on your fork a couple of months ago. Is it working ok for you? Did you need to apply any additional fix?

[umarcor]

Request to be included in 1.0.0. Ref #959.

[jharshman]

API changes are not on the roadmap for the 1.0.0. Bugs, cleanup, and documentation tasks are being targeted for 1.0.0.

[umarcor]

API changes are not on the roadmap for the 1.0.0. Bugs, cleanup, and documentation tasks are being targeted for 1.0.0.

Agree. Precisely, I tried to be careful not to bump PRs with breaking changes.

This PR is the completion (cleanup) of a partial implementation and it is explicitly backwards compatible. I believe that 1.0.0 should include the full implementation, or remove the incomplete parts.

[hilary]

@umarcor It worked for us, although we ended up not continuing down that path.

[umarcor]

This PR is now rebased, to fix conflicts after #1048 was merged.

[danmx]

@umarcor it seems you can merge your PR now

[umarcor]

@umarcor it seems you can merge your PR now

@danmx, yes. This PR is ready to be merged since March 2019. I've been keeping it updated/rebased for 17 months. Unfortunately, I am not a maintainer, so I cannot decide when to merge it.

[umarcor]

• ValidArgs will only be checked if Args is defined. This probably should be mentioned in the documentation when it mentions that ValidArgs is used to check positional args.

I need to check it explicitly, but I wrote the following in the README:

If Args is undefined or nil, it defaults to ArbitraryArgs.

Therfore, my understanding is that ValidArgs will always be checked, if defined, regardless of Args.

• Say I want to have the behaviour of the deprecated OnlyValidArgs, how would I achieve this now? IIUC I would have to set Args: ArbitraryArgs? I find this a little confusing as my intent is to only allow ValidArgs.

You would not need to set it, because it would default to that. If you wanted to define Args explicitly, you'd need to use ArbitraryArgs. I agree that it might be confusing to have Args: ArbitraryArgs and ValidArgs at the same time. However, "Arbitrary" in that context means "any number of arguments". The naming is unfortunate, but it is preserved so that this PR is backwards compatible. If maintainers wanted to introduce a breaking change, I'd be ok with renaming it to AnyNumberOfArgs or any other more descriptive/specific name.

[umarcor]

@marckhouzam I fixed the order of the validations, so that ValidArgs is checked regardless of Args. I also added tests for it. Thanks for catching that!

[mikeschinkel]

Checking on the status of this, please?

I am looking for suggestions on subcommands and it appears this and #842 are needed to provide them, if I read #981 correctly?

[umarcor]

@mikeschinkel the status is the same as explained in https://github.com/spf13/cobra/issues/981#issuecomment-647784636.

[umarcor]

A PR was created with the first three commits from this one: #1426. Then, this PR was cleaned up. Hope that helps move forward...

[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.

[tmc]

Could/should this change take into account ValidArgsFunction?

[marckhouzam]

Could/should this change take into account ValidArgsFunction?

Please see #1298

[umarcor]

@tmc, since ValidArgs and ValidArgsFunction are mutually exclusive, I don't see how to change this to take the latter into account. If you use ValidArgsFunction, it should be handled by the completion logic, as far as I understand. Maybe you can provide a reproducer of what you are trying to achieve?

[umarcor]

Please, remove the stale label from this PR.

Refs:

• https://github.com/spf13/cobra/issues/1496#issuecomment-989131001
• https://github.com/spf13/cobra/issues/1598#issuecomment-1038566503

[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[bot]]

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.