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 missing test and support help options with only the short flag in Help.visibleOptions() #1935

Closed
wants to merge 16 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 4, 2023

An extension of #1929 fixing a problem I run into when writing demo code for #1931.

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

Problem

program
  .option('-h, --host')
  .helpOption('-h')
  .outputHelp(); // throws
lib\option.js:17
    this.required = flags.includes('<'); // A value must be supplied when the option is specified.
                          ^

TypeError: Cannot read properties of undefined (reading 'includes')
    at new Option (lib\option.js:17:27)
    at Command.createOption (lib\command.js:500:12)
    at Help.visibleOptions (lib\help.js:79:26)
    at Help.longestOptionTermLength (lib\help.js:197:19)
    at Help.padWidth (lib\help.js:419:14)
    at Help.formatHelp (lib\help.js:349:30)
    at Command.helpInformation (lib\command.js:1988:19)
    at Command.outputHelp (lib\command.js:2028:32)

Solution

Help.visibleOptions() code has been updated to support help options with only the short flag well.

Tests have been added to cover this and other similar use cases.

ChangeLog

Changed

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

Fixed

  • added missing support for help options with only the short flag

Borrowed from a3f0e28 that was supposed to land in the now-closed tj#1921.
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).
Eliminates the need for the check in other places.

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

aweebit commented Aug 4, 2023

Side note: I don't know if I'm doing it right with the base PR "extensions" and the merges. The commits and file changes look really cluttered now. If it's appropriate and if #1929 can't be merged right now, a good idea might be to create a temporary branch based off #1929 and make it the base for all PRs building on it (#1931, #1934, #1935). This PR would then only contain the last two commits which are the relevant ones, for example, making it much simpler to review the changes.

@aweebit aweebit changed the title Add missing Help.visibleOptions() test and support help options with only the short flag in Help.visibleOptions() Add missing test and support help options with only the short flag in Help.visibleOptions() Aug 4, 2023
@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
@shadowspawn
Copy link
Collaborator

Side note: I don't know if I'm doing it right with the base PR "extensions" and the merges. The commits and file changes look really cluttered now.

With 20/20 hindsight I can confidently say this was not the right approach. 😆

Some parts this have been approved separately, and some parts have been rejected separately. Closing this one as too messy to try and review what is left.

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