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

Add .requiredOption() for mandatory options #1071

Merged
merged 10 commits into from Oct 8, 2019

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Oct 5, 2019

Pull Request

Problem

Some programs have options which are mandatory. This usually means they must be specified on the command line. This is not directly supported by Commander, and is a popular request. (It is often confused with options which have a required value when the option is specified.)

Issues: #230 #1038

Solution

The most popular form for the API (#1038) is to add a method called .requiredOption(). For simplicity it is has the same signature as .option() and supports all the same parameters.

(This new functionality is in addition to the existing support for options with a required value when the option is specified, like '-f,--foo <requiredValue>'.)

Commander will check that each mandatory option has a value after parsing, and display an error if the option value has not been supplied. Requiring a value rather than requiring the option on the command line allows use of environment variables to optionally supply default values.

@shadowspawn
Copy link
Collaborator Author

Pretty tidy now, but I'll leave as a draft for another day in case I think of anything more. I made some improvements today after thinking on it overnight. :-)

I think this will be safe to add to 4.0.0 when (if) approved since it is a separate new feature that should not affect any existing parsing.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Oct 5, 2019

ToDo: add call to .requiredOption to TypeScript test program.

Edit: added calls to .requiredOption to TypeScript test program.

@shadowspawn
Copy link
Collaborator Author

I did a little research into the help conventions for required options. There are common conventions for the short description in the usage with optional options in square brackets, but I didn't see a common convention for the longer list of options.

Plan to leave it up to the user, for now. Obviously the user can change the option description itself. We also support the user customising the "usage" description, which is a good place to show required options as part of the calling signature. e.g.

Usage: foo --host HOST [--debug] [--port PORT]

An interesting lightweight idea I saw in picocli was optional support for prefixing lines with required options with a character (like '*'). This would be very easy to add later. e.g.

Usage: foo --host HOST [--debug] [--port PORT]

  --debug            include debugging information
* --host <name>      mandatory host name
  --port <number     port number

@shadowspawn shadowspawn changed the title WIP: add .requiredOption() for mandatory options Add .requiredOption() for mandatory options Oct 6, 2019
@shadowspawn
Copy link
Collaborator Author

Ready for review. No rush. Feedback welcome.

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.

👍

@shadowspawn
Copy link
Collaborator Author

Thanks @abetomo

I am thinking of doing another prerelease version of 4.0.0 with this change included?

@abetomo
Copy link
Collaborator

abetomo commented Oct 7, 2019

I think it can be included in 4.0.0.
If you want to emphasize the addition of features, you can release it in 4.1.0.

@shadowspawn shadowspawn merged commit 7f9edba into tj:develop Oct 8, 2019
@shadowspawn shadowspawn deleted the feature/required-option branch October 8, 2019 01:15
@shadowspawn shadowspawn added this to the v4.0.0 milestone Oct 8, 2019
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Oct 8, 2019
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants