Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More consistent way of adding command arguments #1490

Merged
merged 36 commits into from Apr 8, 2021

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Mar 27, 2021

Pull Request

Problem

The syntax for adding command arguments which have help descriptions is quite different from the rest of the API, and arguments and options are added quite differently although they could be similar. (Prompted by discussion about offering .choices() for arguments in #1458)

Related: #1458 #1467 #1471

Solution

This builds on PR #1467. With positive feedback about the new method in #1471., switching over "normal" way of adding an argument in README and examples and tests from .arguments() to .argument().

  • .argument(name, description) method
    • c.f. .option(flags, description)
  • addArgument(arg) method for future expansion
    • c.f. .addOption(opt)
  • deprecate cmd.description(cmdDescription, argsDescription)

ChangeLog

  • added: cmd.argument(name, description) for adding command-arguments
  • changed: deprecate second parameter of cmd.description(desc, argDescriptions) for adding argument descriptions (removed from README)
  • changed: Help .visibleArguments() returns array of Argument

@shadowspawn shadowspawn added the semver: major label Mar 27, 2021
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Mar 28, 2021

Allowing no brackets around argument name and defaulting to required option, but not documenting for now.

i.e. foo treated same as <foo>

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Mar 28, 2021

Not documenting .addArgument() in README yet, as not interesting until has some extra behaviours (like .choices()!).

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Mar 28, 2021

Might be done! (Edit: no, was not done!) I am going to have a look at the Argument data property names, but they are consistent with Option and Command. Having a plain data property name like required means we can't have a method called required() in the future (well, without it being a breaking change).

Mentioned property names in #1467 (comment)

Update: keep properties private for now (Like Command.)
Update 2: change back, closely follow example of Option

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Mar 29, 2021

Also I will have another look at the README introduction of the arguments, as the "inline" form with command-name <arg1> [arg2] is the only one that can be used when declaring stand-alone executables from the parent command. .argument() does not replace this. (Not all the patterns work for program vs subcommand, and action handler vs stand-alone executable.)

index.js Show resolved Hide resolved
@shadowspawn shadowspawn marked this pull request as ready for review Apr 5, 2021
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Apr 5, 2021

I have had another go at the README to try and tidily cover the multiple approaches to specifying command-arguments.

@shadowspawn shadowspawn changed the base branch from develop to release/8.x Apr 5, 2021
abetomo
abetomo approved these changes Apr 7, 2021
Copy link
Collaborator

@abetomo abetomo left a comment

8️⃣

@shadowspawn shadowspawn merged commit b286da9 into release/8.x Apr 8, 2021
24 checks passed
@shadowspawn shadowspawn deleted the feature/argument branch Apr 10, 2021
@shadowspawn shadowspawn added this to the v8.0.0 milestone Apr 10, 2021
@shadowspawn shadowspawn added the pending release label Apr 10, 2021
@shadowspawn shadowspawn removed the pending release label Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants