Enhance Option class to allow hiding help, specifying choices, and change how default value displayed in help

[shadowspawn]

Pull Request

There are various requests for additional behaviours for options that are hard to add to the existing Command interface. This is an experiment to see whether turning Option into a richer class will allow adding new features in a tidy way by working directly with the option, rather than indirectly from the command.

Problem

• Feature Request: ability to hide options Feature Request: ability to hide options #1106
• Object default values display JSON under help menu Object default values display JSON under help menu #1283
• Provide Array of permitted values for an option. Provide Array of permitted values for an option. #518

Solution

Enhance Option with support for configuring custom behaviours. Add a created Option to command with .addOption().

Use like:

program
   .addOption(new Option('-s, --secret').hideHelp())
   .addOption(new Option('-t, --timeout <delay>', 'timeout in seconds').default(60, 'one minute'))
   .addOption(new Option('-s, --size <value>', 'drink size').choices(['big', 'little']));

$ node help.js -h
Usage: help [options]

Options:
  -t, --timeout <delay>  timeout in seconds (default: one minute)
  -s, --size <value>     drink size (choices: "big", "little")
  -h, --help             display help for command

ChangeLog

• allow hiding options from help
• allow restricting option arguments to a list of choices
• allow setting how default value is shown in help

[Tbhesswebber]

This looks awesome! I'm not seeing any tests for things being chained together - what is the expected behavior if we provide defaults and hide the help like in the following?

program
   .addOption(new Option('-t, --timeout').hideHelp().default(60, 'one minute'));

It seems to me like anything not shown in help should require a default value - should that require .default to be called?

Also, I realize that I was part of the impetus for this PR and I explicitly asked for a way to hide options, but should there be some way to toggle exposure of the hidden options. Something like giving every program a --advancedHelp flag that shows hidden flags? Maybe hideHelp should take an options object that allows it to accept default values and a boolean representing if it should show in advanced usage?

Thanks so much for all of your work on this and I'm sorry that I'm constantly poking holes in things to see if they pop!

[shadowspawn]

Thanks @Tbhesswebber . No popping noises yet. 🎈

I'm not seeing any tests for things being chained together - what is the expected behavior if we provide defaults and hide the help like in the following?

These calls chain (like when configuring Command), so in your example the option will have a default value and be hidden.

It seems to me like anything not shown in help should require a default value

Why is it different to a non-hidden option? If I don't look in the help, then I may not know what other options exists or whether they have a default value.

but should there be some way to toggle exposure of the hidden options

You can arrange for this yourself by passing false into hideHelp. Say by testing an environment variable to see whether to show everything.

[Tbhesswebber]

Fantastic! I think I just naturally think of all things having defaults within a CLI in order to avoid having to handle multiple ways of tracking values (with the exception of anything that is provided by a config file), so you can ignore that point of mine!

Again, I really appreciate your efforts here!

[shadowspawn]

I made a bunch of tidies since review comment. (A couple related to previous PR, sorry! 😊 )

• remove duplicated code by having .requiredOption() implemented using .option()
• rename parseArgWith to argParser as more conventional name
• improve JSDoc
• describing deprecated features as legacy in the comments, since we won't delete them until there is a good reason
• added @deprecated to TSDoc so get editor feedback (e.g. latest VSCode this week!)

[shadowspawn]

npm run typescript-checkJS is currently showing some errors, including from my latest changes. I will investigate...

[shadowspawn]

Fixed.

[shadowspawn]

describing deprecated features as legacy in the comments, since we won't delete them until there is a good reason

I revisited this today. I was worried "deprecated" meant "we will delete this" and was too strong. But deprecated also can mean "this was a bad idea" or "there is a better way now". I'll go back to calling things deprecated instead of legacy, it is the right word for the functionality we would like people to stop using and is supported with the @deprecated tag.

So I'll make some wording changes but not logic changes, again! Sorry for the churn, but I am pleased to get this sorted out in my mind. I have been wondering how best to talk about old code for a while. 🤔 💡

[shadowspawn]

Done. Back to "deprecated"!

[abetomo]

awesome!

[shadowspawn]

Thanks for review @abetomo , it was a big one! 🎉