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

Warn about obscured help option #1931

Closed

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 3, 2023

An extension of #1929 adding warnings about obscured help option as suggested in #1926 (comment).

Additionally proves the usefulness of storing the Option instance returned from createOption() in the _helpOption property (a change introduced by 9857b1e in #1929), since in the solution, not only the individual flags but also the originally provided flags value needs to be accessed, and in the future it could be the .name(), the .attributeName() or something else pertaining to the help option. It makes no sense to add a property to the Command instance each time such a property is needed, it is way better to simply store the Option instance.

Has been merged with the already approved tiny fixes PR #1930 because the parent #1929 has been merged with it.

Problem

States in which the help options is obscured are supported by the library (see Help.visibleOptions() and the corresponding unit tests), but definitely not desirable.

const desc = 'added via .option()';

var program = new Command();
program
  .option('-h, --help <arg>', desc) // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('-h, --host <arg>', desc)
  .option('--help <arg>', desc) // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('-c, --custom-help <arg>', desc)
  .helpOption('-c, --custom-help') // help option is obscured
  .outputHelp();

// Support for this option combination is currently missing because of a wrong Help.visibleOptions() implementation.
// I will open another PR to address that soon. (Update: Opened #1935)
var program = new Command();
program
  .option('-c, --custom-help <arg>', desc)
  .helpOption('-c') // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('-c, --custom-help <arg>', desc)
  .helpOption('--custom-help') // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('-c <arg>', desc)
  .option('--custom-help <arg>', desc)
  .helpOption('-c, --custom-help') // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('-c <arg>', desc)
  .helpOption('-c') // help option is obscured
  .outputHelp();

var program = new Command();
program
  .option('--custom-help <arg>', desc)
  .helpOption('--custom-help') // help option is obscured
  .outputHelp();

Solution

Add warnings when such a state is reached similar to those in #1915 and #1917.

ChangeLog

Added

  • warnings about obscured help option

Changed

  • (from #1929) Breaking: use .createOption() in .helpOption() to support custom option flag extraction

Peer PRs

…solving similar problems

Warnings need to be consistent with…

Requires changes when merged with…

Analogous to 3c94dfd in tj#1933 for version option.

Storing the Option instance of the help option instead of just its
flags (_helpShortFlag, _helpLongFlag) is meaningful for cases when other
properties of the instance need to be accessed (not currently the case,
but could be in the future).
_registerOption() has been borrowed from deb3bdf in tj#1923.
Watch out for merge conflicts!
@shadowspawn
Copy link
Collaborator

A quick reaction. I may not accept this PR, or at least not to "Warn about obscured help option". The zero-config obscuring was added in #1247 and raised in #645. The ability to explicitly suppress the built-in help option was added later in #1325 and perhaps the obscuring is no longer as appropriate, so I'll think about it a bit.

Separately, storing the help option configuration in an actual Option is something I considered again while working on #1910. I have not had a deep look yet myself. (I resisted attempting it in the same PR.)

Eliminates the need for the check in other places.

(cherry picked from commit a12f5be)
@aweebit
Copy link
Contributor Author

aweebit commented Aug 4, 2023

(I resisted attempting it in the same PR.)

I realized only later that I had had to do the change in the base #1929, that is why there is the empty 371751e.

@shadowspawn shadowspawn added the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 8, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
@shadowspawn
Copy link
Collaborator

I think the commit specific to the title for this PR is d03566b

The current behaviour evolved out of a sort of auto-configuration for the implicit help option, which slowly became more robust as doing other work. I suspect the behaviour is more appropriate for simpler programs where the author uses -h say and we just get out of the way with the default help option.

That leads to an interesting thought that could treat explicit and implicit help options differently. But not solving a problem I am invested in for now, and behaviour is as intended.

(I have lost track of how many of your PRs have the help configuration refactored into a help option. That is something I am interested in as a concept, but not urgently, and may refer back to this in the future.)

@shadowspawn shadowspawn removed the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 20, 2023
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