added addArguments method

[Niryo]

Pull Request

Problem

inconsistent arguments and options syntax

Solution

introducing new addArgument function

ChangeLog

New command method: addArgument() and .argument()

[Niryo]

@shadowspawn This is a draft with the mvp for addArguments method. can you take a look and tell me what you think and what are the chances for a merge, before I continue to tests, docs and qa?
If this PR looks fine, i'll continue with adding choices.

[shadowspawn]

Modified code less than I expected for first draft, which is nice for reading the diff. Sticking to the features which are already supported (argument name and argument description) keeps it simpler.

what are the chances for a merge, before I continue to tests, docs and qa?

50%
Stick to the code for now, I'm going to add a few comments.

I am actually liking this more by leaving out the default value and custom coerce functions for now. There is room in the syntax to add them later if there is demand.

[shadowspawn]

I wouldn't release .addArgument() without a matching convenience routine on the Command for .argument(), like .option() and .addOption().

[Niryo]

@shadowspawn do you think we should deprecate .arguments() in order to keep the api small? I think that the syntax of .argument() is more elegant and robust (no need to specify the description separately using hash and open the door for typos)

[shadowspawn]

@shadowspawn do you think we should deprecate .arguments() in order to keep the api small?

At least to some extent, yes. I am thinking the description hash would get dropped from the README, but maybe still mention briefly that can use .arguments('<a> <b> [c...]') as a shortcut for adding multiple arguments. The example code in the README would switch to using .argument().

[Niryo]

I wouldn't release .addArgument() without a matching convenience routine on the Command for .argument(), like .option() and .addOption().

What about the tests? Can I convert the tests that use arguments to tests that use argement and add argument, in order to verify that everything is working? It will be more convenient to change the tests instead of duplicate everything

[shadowspawn]

The old functionality is not being removed (yet), so the existing tests are still relevant to prevent regressions.

[Niryo]

@shadowspawn this pr is ready for review

[shadowspawn]

Oh drat, I have been making comments but they were not visible yet as I had not started a review. I wondered why you were ignoring them!

[shadowspawn]

FYI: opened an issue to poll reactions to API changes in #1471

[shadowspawn]

One thing I have been wondering about but undecided is whether .argument() should update an existing argument with same name and signature. This would allow adding descriptions after using a shortcut/old method. For example:

program
  .command('build <target>')
  .argument('target', 'source file for compilation')

[Niryo]

It's quite redundant because if someone will add the argument expression in order to add description, he can just remove the arg from the command expression on the way.

[Niryo]

I think all comments are resolved

[shadowspawn]

I had a go at consistently using Argument class for _args and removing the previous/intermediate untyped argDetails object. A second commit adds separate processing for legacy argument descriptions and new descriptions.

I added a draft PR to your branch. It led me to wonder whether to be more lenient with what should be done with incorrectly formatted argument. e.g.

.argument('foo')
# could be treated same as
.argument('<foo>')

[Niryo]

I think that the direction we should go to should be towards more explicit syntax, for example I would prefer using something like new Argument('bla').required(), instead of remembering which means required - <> or [].
Also, with the explicit syntax you get auto complete..

[shadowspawn]

The description from arguments.js would be better, 'password for user, if required'.

[shadowspawn]

The test naming convention being used is:

• when something then result

[shadowspawn]

I think that the direction we should go to should be towards more explicit syntax, for example I would prefer using something like new Argument('bla').required(), instead of remembering which means required - <> or [].

I am comfortable with <foo> as the compact form since it matches the help, but supporting long form is good too. There are some routines on the Option class, but not for required as the interaction of the possibilities is more complicated. Simpler with Argument so could support required, optional, and variadic. Also Command and Option both have .name() so probably should add that.

We are using two patterns, with either getter/setter depending on whether an argument is passed, or setter with no argument or optional argument. The pure setters sometimes have a verb like hideHelp or allowUnknownOption.

Option has required and optional and variadic so using those names for the data properties is consistent. But that means other names for the setters.

[Niryo]

Anyways, it was just a general thought, it's not really important for this PR, we'll discuss it further in the future if I want to add aa required prop:)

[Niryo]

@shadowspawn The PR is ready on my side, if you need something else add more comments

[shadowspawn]

The two comments I had left are:

• added addArguments method #1467 (comment)
• added addArguments method #1467 (comment)

(But they are small and I can fix them up later.)

[shadowspawn]

I am thinking of pulling this onto a branch and dong some more work on it, and including in Commander v8 in June. The changes to how non-conforming arguments are processed/rejected make it a breaking change for a small subset of users. (e.g. <foo> and [bar] stay the same but what-did-you-expect changes)

[shadowspawn]

Approving for merging onto a feature branch (for some further work).