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 Releasing requires a major version bump, not backwards compatible label Mar 27, 2021
@shadowspawn
Copy link
Collaborator Author

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

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 April 5, 2021 02:14
@shadowspawn
Copy link
Collaborator Author

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 April 5, 2021 21:41
Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8️⃣

@shadowspawn shadowspawn merged commit b286da9 into release/8.x Apr 8, 2021
@shadowspawn shadowspawn deleted the feature/argument branch April 10, 2021 00:28
@shadowspawn shadowspawn added this to the v8.0.0 milestone Apr 10, 2021
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Apr 10, 2021
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet 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 Releasing requires a major version bump, not backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants