Support validation of positional arguments #284

dnephin · 2016-05-27T05:00:36Z

Fixes #42
Branched from #120

Adds a new field Args that allows a user to define the expected positional arguments that a command should receive. If a command doesn't receive the expected args it returns an error.

By accepting a validation function users can create their own custom validation for positional args. This PR includes validators for the most common cases.

deads2k · 2016-05-27T15:13:51Z

args.go

+}
+
+// NoArgs returns an error if any args are included
+func NoArgs(cmd *Command, args []string) error {


Can a noarg command have subcommands? This seems like the trigger I wanted in #285, right?

Yes, subcommand lookup happens before this is called in Find, so these arg handling functions only take care of non-subcommand args. I believe this NoArgs option is what you want in #285.

dnephin · 2016-05-27T23:06:37Z

There's a problem with this PR. Arg validation happens before the --help flag is checked, so they can report errors incorrectly when --help is used.

I think the order of these operations needs to be adjusted.

dnephin · 2016-05-31T02:36:52Z

Ok, that issue should be fixed. Now legacy arg validation happens at the same time (before any flags are parsed), but any new arg validation will happen after flag parsing, so that flags like --help will behave correctly.

I updated the tests to catch this case.

stevekuznetsov · 2016-06-02T12:57:39Z

cobra_test.go

@@ -513,6 +523,11 @@ func TestSubCmdTakesArgs(t *testing.T) {
 func TestCmdOnlyValidArgs(t *testing.T) {
 	result := noRRSetupTest("echo times one two five")

+	if result.Error == nil {
+		t.Errorf("Expected an error")
+		t.FailNow()


The testing package provides:

t.Fatal("Expected an error")

as a convenience method for wrapping the two calls you're making here.

Thanks. I've made that change. It did seem strange that I had to call two functions, but somehow I missed this.

dnephin · 2016-06-20T19:09:58Z

This PR is in use by https://github.com/docker/docker to validation positional args.

By using a function instead of an enum for the Args field, it allows us to do things like validate the format of args before they are passed to any Run function.

rileytg · 2016-08-07T16:12:27Z

Am I understanding correctly?

In addition to the documented features, you can now validate arguments in a custom PositionalArgs type func passed to Args:.

contrived example (imagine if instead of :

func validColorArgs(cmd *cobra.Command, args []string) error {
  if err := cli.RequiresMinArgs(1)(cmd, args); err != nil {
    return err
  }
  if myapp.IsValidColor(args[0]) {
     return nil
  }
  return fmt.Errorf("Invalid color specified: %s", args[0])
}

dnephin · 2016-08-07T16:31:37Z

@rileytg yup, that's correct. You can write custom validation functions for the positional args.

rileytg · 2016-08-10T17:40:22Z

what do you think about changing the language to express something along the lines of: (im thinking maybe keep it like urs and add a custom to the end of the list with my example.)

Positional and Custom Arguments

Validation of positional arguments can be specified using the Args field. A custom validator can be provided like this:

Args: func validColorArgs(cmd *cobra.Command, args []string) error {
  if err := cli.RequiresMinArgs(1)(cmd, args); err != nil {
    return err
  }
  if myapp.IsValidColor(args[0]) {
     return nil
  }
  return fmt.Errorf("Invalid color specified: %s", args[0])
}

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

dnephin · 2016-08-10T18:40:38Z

That sounds good. I'll update the PR with that change

Julio-Guerra · 2016-09-04T21:26:10Z

What about the help message of such commands? It would be great to automatically generate command [flags] arg1 arg2 arg3, etc. according to Args or whatever. Mostly because of [flags] which is already automatically concatenated to the Use field. So we can't simply put arg1 arg2 arg3 in the Use field, otherwise we end up with command arg1 arg2 arg3 [flags] instead of command [flags] arg1 arg2 arg3.

dnephin · 2016-09-16T14:01:30Z

I find the [flags] placement to be unreliable in general, and I disable it by setting a custom template. The problem you describe already exists today with a use line. You can fix it by setting the use line to command [flags] arg1 arg2 arg3 explicitly.

dnephin · 2016-09-16T14:05:30Z

Rebased and updated the README text as suggested

jwhitcraft · 2017-01-24T20:52:59Z

I would really love this feature to be merged, so I don't have to use the fork and be able to get all other fixes.

@dnephin @eparis what is the status of this?

dnephin · 2017-01-25T19:14:37Z

Still waiting for feedback from the project maintainers.

This code is being used in docker/docker (as of 1.13).

I pulled the latest from spf13/master into my fork, so you could use that for now if you want.

jwhitcraft · 2017-01-26T16:06:06Z

@dnephin, i just pulled it down and it seems your master branch is missing the commits from this branch. Could you rebase this branch with master so i can just use this branch from your fork?

Thanks!

dnephin · 2017-01-26T16:30:18Z

The master branch has these changes, it just doesn't have the same commits. I think they were rebased from a different branch, so the commit ids are different.

kofalt · 2017-02-27T19:42:15Z

cobra_test.go

@@ -109,6 +113,16 @@ var cmdRootSameName = &Command{
 	Long:  "The root description for help",
 }

+var cmdRootTakesArgs = &Command{
+	Use:   "root-with-args [random args]",
+	Short: "The root can run it's own function and takes args!",


s/it's/its

harsimranmaan · 2017-03-30T06:54:54Z

@spf13 @dnephin Is this still waiting on something?

vadim-su · 2017-04-09T01:23:13Z

@dnephin what is the status of the PRs now?

dnephin · 2017-04-10T16:51:22Z

Status hasn't changed. Waiting on someone to accept or reject, or provide more feedback about how to get it accepted.

n10v · 2017-04-22T10:09:24Z

@dnephin what do you think of it:

But maybe Args field will be called as CheckArgs or somehow else so it will be clearer to understand the purpose of function.
And I think built-in validators should be above custom.
Also IMHO it's more beautiful to conclude name of built in functions in backticks.

dnephin · 2017-04-22T21:25:05Z

I've made those changes to the README.

I'm not really a fan of CheckArgs. Check them for what? ValidateArgs maybe, but there is already a ValidArgs which is a bit too similar.

n10v · 2017-04-22T22:06:15Z

Ok, 'Args' is good too
Actually I like this PR. It adds really needed this feature and fix bug, to that there are many complains (#42).
We should only wait for @spf13 agreement

harsimranmaan · 2017-04-28T06:57:41Z

Thanks @BoGeM and @dnephin for driving this forward. Hopefully, @spf13 would agree for this merge now. Much appreciated.

n10v · 2017-05-12T12:42:45Z

Please solve merge conflicts.
We should merge it when @spf13 will be free.

n10v · 2017-05-16T20:19:20Z

@dnephin I solved the merge conflicts by myself. Please copy all code from my branch (git pull https://github.com/bogem/cobra.git dnephin-define_args) and paste to yours. After you push conflict solutions, I will write directly to @spf13 so he will say his (final) decision about this PR.
I will not commit to master (except of bug fixes) till we merge it.

…ry arguments Check that arguments are in ValidArgs If a command defined cmd.ValidArgs check that the argument is actually in ValidArgs and fail if it is not.

Fix some typos in README and comments. Move arg validation to after flag validation so that the help flag is run first. Pass the same args to ValidateArgs as the Run methods receive. Update README. Signed-off-by: Daniel Nephin <[email protected]>

dnephin · 2017-06-05T19:17:17Z

rebased

harsimranmaan · 2017-06-07T01:17:18Z

@BoGeM @spf13 The ball is in your court now. Thanks @dnephin .

kofalt · 2017-07-24T15:14:25Z

Thanks @BoGeM and @dnephin! 🎉

notjames · 2017-07-26T10:52:08Z

I know that this is coming from left field a little, however, I'm very new to Go and I'm trying to port an internal application and am currently very stuck primarily because the documentation simply does not make sense with regard to custom argument validation. I've been through this entire issue and the README.md, which briefly mentions it, but gives NO context about where it should go, how it should properly be used, etc, and everything I try ends up being an error. Would someone for the love of all that is good PLEASE explain how to use Args: and friends IE custom argument validation? @rileytg ?

For instance, the docs say we can use this example (and I'm only supposing this belongs in the &cobra.command() call:

func validColorArgs(cmd *cobra.Command, args []string) error {
  if err := cli.RequiresMinArgs(1)(cmd, args); err != nil {
    return err
  }
  if myapp.IsValidColor(args[0]) {
     return nil
  }
  return fmt.Errorf("Invalid color specified: %s", args[0])
}

So, I used that. I get an error. syntax error: unexpected validArgs, expecting ( and apparently use of Args takes Args PositionalArgs so I will need to look that up. However, none of this is documented very well...I think.

Tangentially, the documentation for this project tries to be good, but it is seriously lacking some important details methinks. It almost seems like nobody is keeping it updated or at the very least is keeping it updated only minimally.

For instance, I thought someone hijacked the documentation when mentioning even the ability for custom arg validation because when I tried to use it, my IDE screamed that it wasn't even available in the struct. Turns out, after reading this issue and seeing the change was just merged a couple of days ago that the problem was simply an outdated library on my system. Kudos for adding something about it at all in the documentation, but the details about how to properly use it are completely wrong or just missing.

How are developers supposed to rely on this library if we can't track the changes or use the library reliably? Sorry for the rant. It's almost 4 AM so...it's just that I've lost a lot of time trying to figure this out and it seems like I shouldn't have had to.

n10v · 2017-07-26T12:34:38Z

@notjames Sorry for inconvenience and this feature is documented really bad. I will make a PR now to fix this issue and we can discuss it there.
Thank you for your notice!

Positional argument validation was added to spf13/cobra in spf13/cobra#284, but our cli still relies on custom validation in each of the "run" functions. This validation is unstandardized and overly permissive in certain cases. This change fixes this by replacing our custom logic with validation expressed using Cobra's `PositionalArgs` validators. Release note: None

dnephin force-pushed the define_args branch from c650728 to 2d158e1 Compare May 27, 2016 05:01

dnephin mentioned this pull request May 27, 2016

allow suggestions for non-root commands #285

Closed

deads2k reviewed May 27, 2016
View reviewed changes

stevekuznetsov reviewed Jun 2, 2016
View reviewed changes

deads2k mentioned this pull request Jun 2, 2016

suggest can-i for what-can-i-do openshift/origin#9055

Closed

dnephin force-pushed the define_args branch from f2c7ddd to 5f3d6c4 Compare June 2, 2016 21:43

dnephin mentioned this pull request Jun 23, 2016

Command allowing both arguments and subcommands #295

Closed

dnephin mentioned this pull request Jul 1, 2016

If no subcommand, how to get args? #42

Closed

dnephin force-pushed the define_args branch from c76d2fa to 631cea0 Compare September 16, 2016 14:05

dnephin mentioned this pull request Oct 16, 2016

Why docker uses the trick that cloning dnephin/cobra instead of spf13/cobra? moby/moby#27415

Closed

dnephin mentioned this pull request Feb 27, 2017

Documenting a positional argument and suppressing [flags] when appropriate #395

Closed

kofalt reviewed Feb 27, 2017

View reviewed changes

dnephin force-pushed the define_args branch from 631cea0 to d4efd35 Compare February 28, 2017 17:03

dnephin force-pushed the define_args branch from 709fb80 to 845bf47 Compare April 19, 2017 15:10

n10v mentioned this pull request Apr 22, 2017

use root command if no other command matches and root is runnable #301

Closed

dnephin force-pushed the define_args branch 2 times, most recently from 85279b9 to fb31500 Compare April 22, 2017 21:23

harsimranmaan approved these changes May 4, 2017

View reviewed changes

n10v mentioned this pull request May 9, 2017

Things we want in v2 #440

Closed

eparis and others added 2 commits June 5, 2017 14:22

Allow commands to explicitly state if they do, or do not take arbitra…

0a19163

…ry arguments Check that arguments are in ValidArgs If a command defined cmd.ValidArgs check that the argument is actually in ValidArgs and fail if it is not.

dnephin force-pushed the define_args branch from fb31500 to 7987bcc Compare June 5, 2017 19:17

dnephin mentioned this pull request Jun 22, 2017

how to enforce the number of arguments required? #471

Closed

n10v merged commit f20b4e9 into spf13:master Jul 23, 2017

This was referenced Jul 24, 2017

Allow commands to specify if they take arbitrary arguments #120

Closed

Ability to define arguments as well (like flags) #378

Open

nvanbenschoten mentioned this pull request Apr 2, 2018

cli: use Cobra's positional argument validation cockroachdb/cockroach#24399

Merged

patrick-motard mentioned this pull request Sep 14, 2018

ExactArgs + ValidArgs + OnlyValidArgs #745

Closed

umarcor mentioned this pull request Mar 27, 2022

deprecate ExactValidArgs and test combinations of args validators #1643

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support validation of positional arguments #284

Support validation of positional arguments #284

dnephin commented May 27, 2016

deads2k May 27, 2016

dnephin May 27, 2016

dnephin commented May 27, 2016

dnephin commented May 31, 2016

stevekuznetsov Jun 2, 2016

dnephin Jun 2, 2016

dnephin commented Jun 20, 2016

rileytg commented Aug 7, 2016

dnephin commented Aug 7, 2016

rileytg commented Aug 10, 2016

dnephin commented Aug 10, 2016

Julio-Guerra commented Sep 4, 2016

dnephin commented Sep 16, 2016

dnephin commented Sep 16, 2016

jwhitcraft commented Jan 24, 2017

dnephin commented Jan 25, 2017

jwhitcraft commented Jan 26, 2017

dnephin commented Jan 26, 2017

kofalt Feb 27, 2017 •

edited

Loading

harsimranmaan commented Mar 30, 2017

vadim-su commented Apr 9, 2017

dnephin commented Apr 10, 2017

n10v commented Apr 22, 2017

dnephin commented Apr 22, 2017

n10v commented Apr 22, 2017

harsimranmaan commented Apr 28, 2017

n10v commented May 12, 2017 •

edited

Loading

n10v commented May 16, 2017 •

edited

Loading

dnephin commented Jun 5, 2017

harsimranmaan commented Jun 7, 2017 •

edited

Loading

kofalt commented Jul 24, 2017

notjames commented Jul 26, 2017 •

edited

Loading

n10v commented Jul 26, 2017 •

edited

Loading

Support validation of positional arguments #284

Support validation of positional arguments #284

Conversation

dnephin commented May 27, 2016

deads2k May 27, 2016

Choose a reason for hiding this comment

dnephin May 27, 2016

Choose a reason for hiding this comment

dnephin commented May 27, 2016

dnephin commented May 31, 2016

stevekuznetsov Jun 2, 2016

Choose a reason for hiding this comment

dnephin Jun 2, 2016

Choose a reason for hiding this comment

dnephin commented Jun 20, 2016

rileytg commented Aug 7, 2016

dnephin commented Aug 7, 2016

rileytg commented Aug 10, 2016

Positional and Custom Arguments

dnephin commented Aug 10, 2016

Julio-Guerra commented Sep 4, 2016

dnephin commented Sep 16, 2016

dnephin commented Sep 16, 2016

jwhitcraft commented Jan 24, 2017

dnephin commented Jan 25, 2017

jwhitcraft commented Jan 26, 2017

dnephin commented Jan 26, 2017

kofalt Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

harsimranmaan commented Mar 30, 2017

vadim-su commented Apr 9, 2017

dnephin commented Apr 10, 2017

n10v commented Apr 22, 2017

dnephin commented Apr 22, 2017

n10v commented Apr 22, 2017

harsimranmaan commented Apr 28, 2017

n10v commented May 12, 2017 • edited Loading

n10v commented May 16, 2017 • edited Loading

dnephin commented Jun 5, 2017

harsimranmaan commented Jun 7, 2017 • edited Loading

kofalt commented Jul 24, 2017

notjames commented Jul 26, 2017 • edited Loading

n10v commented Jul 26, 2017 • edited Loading

kofalt Feb 27, 2017 •

edited

Loading

n10v commented May 12, 2017 •

edited

Loading

n10v commented May 16, 2017 •

edited

Loading

harsimranmaan commented Jun 7, 2017 •

edited

Loading

notjames commented Jul 26, 2017 •

edited

Loading

n10v commented Jul 26, 2017 •

edited

Loading