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.

Sign up for GitHub

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

Jump to bottom

fix: assignment of negate property #1302

Closed
wants to merge 1 commit into from
Closed

fix: assignment of negate property #1302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ class Option {
this.required = flags.indexOf('<') >= 0; // A value must be supplied when the option is specified.
this.optional = flags.indexOf('[') >= 0; // A value is optional when the option is specified.
this.mandatory = false; // The option must have a value after parsing, which usually means it must be specified on command line.
this.negate = flags.indexOf('-no-') !== -1;
this.negate = flags.indexOf('--no-') !== -1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should test for it being at the start of the string. The other places in the code use a regular expression, but checking the index would be fine here I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this?

Suggested change
this.negate = flags.indexOf('--no-') !== -1;
this.negate = flags.startsWith('--no-');

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better! (We moved away from using index to using routines in #1264 which is on the develop branch.)

const flagParts = flags.split(/[ ,|]+/);
if (flagParts.length > 1 && !/^[[<]/.test(flagParts[1])) this.short = flagParts.shift();
this.long = flagParts.shift();
Expand Down
9 changes: 9 additions & 0 deletions tests/options.opts.test.js
View file
Open in desktop
Expand Up @@ -70,4 +70,13 @@ describe.each([true, false])('storeOptionsAsProperties is %s', (storeOptionsAsPr
program.parse(['node', 'test']);
expect(program.opts()).toEqual({ pepper: pepperDefault });
});

test('when option with name including no- is specified then it should not have default value as true', () => {
const program = new commander.Command();
program.storeOptionsAsProperties(storeOptionsAsProperties);
program
.option('--module-no-parse <value>', 'test description');
program.parse(['node', 'test']);
expect(program.opts()).toEqual({});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a test.

Testing .opts() being the empty object is a little fragile as .opts() has some subtle differences between storeOptionsAsProperties true/false, but this approach does avoid needing to know what property name the flag turned into. Good enough for now!

});
});