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

Enforce correct option usage and support custom option flag and name extraction in helpOption() #1926

Closed
wants to merge 5 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 2, 2023

Pull Request

Problem

var program = new Command();
var helpOption = program.createOption(program._helpFlags, program._helpDescription);
var helpOptionName = helpOption.attributeName();
console.log(helpOptionName); // 'help'
program.setOptionValue(helpOptionName, 123); // allowed!

var program = new Command();
program.option('--my-help');
program.helpOption('--my-help'); // allowed!

var program = new Command();
program.setOptionValue('myHelp', 123);
program.helpOption('--my-help'); // allowed!

var program = new Command();
program.version('1.0.0');
console.log(program.version()); // '1.0.0'
var versionOption = program.createOption(program._versionOptionName);
var versionOptionName = versionOption.attributeName();
console.log(versionOptionName); // 'version'
program.setOptionValue(versionOptionName, '1.0.1'); // allowed!
console.log(program.version()); // still '1.0.0'

var program = new Command();
program.option('--my-version');
program.version('1.0.0', '--my-version'); // allowed!

var program = new Command();
program.setOptionValue('myVersion', 123);
program.version('1.0.0', '--my-version'); // allowed!

var program = new Command();
program.setOptionValue('foo', 'bar');
console.log(program.getOptionValue('foo')); // 'bar'
console.log(program.opts()); // { foo: 'bar' }
program.storeOptionsAsProperties(); // allowed!
console.log(program.getOptionValue('foo')); // undefined
console.log(program.opts()); // {}

Solution

Throw errors in all demonstrated cases (partially borrowed from a5afe93 in the now-closed #1921).

Additionally, the code for the help option has been refactored to support custom option flag and name extraction mechanisms defined in subclasses. To achieve that, an Option instance returned from a createOption() call is used for extraction in helpOption() (the same is currently done in version()), which is now called in the constructor and copyInheritedSettings().

The first two commits are tiny improvements to the code that have been made while working on the solution and don't really belong anywhere else.

ChangeLog

Changed

  • Breaking: throw errors when trying to set values for the help and version options
  • Breaking: throw error when trying to add help or version option with .opts() name already in use
  • Breaking: throw error when calling .storeOptionsAsProperties() after setting an option value
  • Breaking: use .createOption() in .helpOption() to support custom option flag and name extraction

Fixed

  • falsy parameter value handling in .addHelpCommand()

Peer PRs

@aweebit aweebit changed the title Enforce correct option usage Enforce correct option usage and support custom option flag and name extraction in helpOption() Aug 2, 2023
@aweebit aweebit marked this pull request as draft August 2, 2023 20:33
@aweebit
Copy link
Contributor Author

aweebit commented Aug 2, 2023

Help.visibleOptions() implementation suggests that option conflicts should not be considered a problem. Gonna make some changes to take that into consideration.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 2, 2023

Superseded by #1927 #1928 #1929 #1930 for now.

As for the main matter of this PR, namely conflicting options: it might be a good idea to consider using console.warn() to inform the user about the conflicts. Update: Implemented in #1931.

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

1 participant