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

Allow readonly array as parameter of .choices() #1667

Merged
merged 5 commits into from Jan 4, 2022
Merged

Allow readonly array as parameter of .choices() #1667

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e48b469
Allow readonly array as parameter of .choices()
ouuan Dec 30, 2021
780cc35
Add tests for .choices() typings
ouuan Dec 31, 2021
42e938d
Take a defensive copy of .choices argument to make sure it's readonly
ouuan Dec 31, 2021
4ef9a43
Expand choices tests to cover readonly contract (#1)
shadowspawn Jan 1, 2022
1ee9790
Use the defensive copy of the parameter of .choices() in parseArg
ouuan Jan 1, 2022
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 lib/argument.js
View file
Open in desktop
Expand Up @@ -97,7 +97,7 @@ class Argument {
*/

choices(values) {
this.argChoices = values;
this.argChoices = values.slice();
this.parseArg = (arg, previous) => {
if (!values.includes(arg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The uses of values in the parse function should be replaced with this.argChoices now they may be different.

throw new InvalidArgumentError(`Allowed choices are ${values.join(', ')}.`);
Expand Down
2 changes: 1 addition & 1 deletion lib/option.js
View file
Open in desktop
Expand Up @@ -117,7 +117,7 @@ class Option {
*/

choices(values) {
this.argChoices = values;
this.argChoices = values.slice();
this.parseArg = (arg, previous) => {
if (!values.includes(arg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The uses of values in the parse function should be replaced with this.argChoices now they may be different.

throw new InvalidArgumentError(`Allowed choices are ${values.join(', ')}.`);
Expand Down
4 changes: 2 additions & 2 deletions typings/index.d.ts
View file
Open in desktop
Expand Up @@ -61,7 +61,7 @@ export class Argument {
/**
* Only allow argument value to be one of choices.
*/
choices(values: string[]): this;
choices(values: readonly string[]): this;

/**
* Make argument required.
Expand Down Expand Up @@ -128,7 +128,7 @@ export class Option {
/**
* Only allow option value to be one of choices.
*/
choices(values: string[]): this;
choices(values: readonly string[]): this;

/**
* Return option name.
Expand Down
2 changes: 2 additions & 0 deletions typings/index.test-d.ts
View file
Open in desktop
Expand Up @@ -376,6 +376,7 @@ expectType<commander.Option>(baseOption.hideHelp(false));

// choices
expectType<commander.Option>(baseOption.choices(['a', 'b']));
expectType<commander.Option>(baseOption.choices(['a', 'b'] as const));

// name
expectType<string>(baseOption.name());
Expand Down Expand Up @@ -404,6 +405,7 @@ expectType<commander.Argument>(baseArgument.argParser((value: string, previous:

// choices
expectType<commander.Argument>(baseArgument.choices(['a', 'b']));
expectType<commander.Argument>(baseArgument.choices(['a', 'b'] as const));

// argRequired
expectType<commander.Argument>(baseArgument.argRequired());
Expand Down