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

Use createOption() in helpOption() to support custom option flag extraction (+ various improvements) #1929

Closed
wants to merge 12 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 2, 2023

Cherry-picked from #1926 for better separation of concerns.

Serves as the base for other PRs dealing with help options (#1931, #1934, #1935) and includes various improvements relevant for all of them. For reasoning behind those improvements, see the commit descriptions.

Has been merged with the already approved tiny fixes PR #1930 because a change introduced there would cause an obscure merge conflict if merged with this PR later.

Problem

Unlike version() using an Option instance returned from createOption() to calculate the version option's names, helpOption() does not call createOption() to calculate the help option's short and long flag and instead simply uses the splitOptionFlags() helper function.

class MyOption extends Option {
  constructor(flags, description) {
    super(flags, description);
    this.long = this.long && `--my-${this.long.slice(2)}`;
  }
}

class MyCommand extends Command {
  createOption(flags, description) {
    return new MyOption(flags, description);
  }
}

const command = new MyCommand();
command.version('1.0.0');
console.log(command._versionOptionName); // '--my-version' as expected
command.helpOption('--custom-help');
console.log(command._helpLongFlag); // '--custom-help' instead of '--my-custom-help'

Solution

Call createOption() from helpOption() and store the returned Option instance in a _helpOption property of the Command instance.

Do not store the individual flags in the _helpShortFlag and _helpLongFlag properties anymore, access them via _helpOption.short and _helpOption.long instead. The reasoning behind this change is the following: in the future, there could be need to access not only the individual flags, but also the originally provided flags, 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.

ChangeLog

Changed

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

Peer PRs

…solving similar problems

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).
@aweebit aweebit changed the title Use createOption() in helpOption() to support custom option flag extraction Use createOption() in helpOption() to support custom option flag extraction (+ various improvements) Aug 4, 2023
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 noticed the tests now added in 7335a9c were missing because #1934 did not fail despite not handling obscured help flags correctly.

I am sorry for making this PR a mess, this one should've only included the changes described in the Solution section, and there should've been a different PR based off this one with all the other improvements for #1931, #1934, #1935 to build upon. Unfortunately, it is too late to change this now because too much depends on this PR.

@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 aweebit force-pushed the feature/createOption-in-helpOption branch from fad505b to 513a4b5 Compare August 13, 2023 01:29
@shadowspawn
Copy link
Collaborator

Using a help option rather than tracking help properties separately is something I am somewhat interested in, but more to see than to action for now. So may be referring back to this in 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