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

Implement allowNegateOption. #1355

Closed
wants to merge 5 commits into from
Closed

Implement allowNegateOption. #1355

wants to merge 5 commits into from

Conversation

mshima
Copy link
Contributor

@mshima mshima commented Sep 12, 2020

Pull Request

Fixes #1343

Problem

  • We are currently using commander with unknown option parsing using meow that allows --no- automatically.
  • We implement ~25 sub commands each providing their options.
  • Almost every option contains 3 states: false, true and default (saved value).
  • Adding a negate option to every option is not an option:
    • help will be too big
    • not every option is added the same way, some are generated, some has to be coded.

Solution

  • Add allowNegateOption() to Options.
  • Add implicitNegateOptions() to Command that will set allowNegateOption() at Options.
  • Fallback parsing --no-foo to --foo with false value if no --no-foo option was found.

ChangeLog

index.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

Some initial comments. I am not sure about this.

An implicit negate for a single option is interesting, but overlaps with the current implementation. The original implementation assumed there would be just one of positive or negative option, but #795 added the ability to have both work together.

Turning on implicit negate options for a whole command is quite intrusive. Every option will have to cope with the option value potentially being false.

The implicit negate should probably appear in the help description.

@mshima mshima changed the title Implement implicitNegateOptions. Implement allowNegateOption. Sep 12, 2020
@mshima
Copy link
Contributor Author

mshima commented Sep 12, 2020

Took your addOption from #1331.

@shadowspawn
Copy link
Collaborator

Somewhat related issues:

@shadowspawn
Copy link
Collaborator

shadowspawn commented Sep 26, 2020

There are two separate ideas in the description and original code for this PR:

  • adding automatic support for --no
  • having single option support for --no (rather than adding a separate --no option)

Both are interesting, but I would like more interest and use cases from the community before adding either. At the moment the use case is to match a specific existing implementation.

The single option support for --no has a disadvantage that is overlaps with the existing support, so needs to offer some compelling value to justify supporting multiple approaches.

The code changes in parseOptions in this PR are reasonably tidy. To match historical usage, the custom option processing function is only called on option-arguments and not when the option is used in a boolean way. The existing handling passes in null as the value, and tests for that in the listener. To get similar behaviour with the added --no code the listener could perhaps check for a boolean value in addition to null, or instead of null.

There are major changes to the options code coming in Commander v7 and the changes have now landed on the release/7.x branch. Have a look at what is possible and what is missing there.

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.

2 participants