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

change option default value type to unknown #1725

Closed
wants to merge 2 commits into from

Conversation

wwwenjie
Copy link
Contributor

Problem

current the type of option default value is string | boolean, which will cause warning in IDE since it can be number or other.

Solution

this PR change the type from string | boolean to unknown to make sure default value can accpet other types.

ChangeLog

change option default value type to unknown

@shadowspawn
Copy link
Collaborator

See also: #1721

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 13, 2022

You are right the default argumentment can technically be anything, but the .option() method is overloaded. Changing the default argument type to unknown (or any) means basically no type checking at all after the flags and description.

The current definitions (after #1721) are:

option(flags: string, description?: string, defaultValue?: string | boolean | string[]): this;
option<T>(flags: string, description: string, fn: (value: string, previous: T) => T, defaultValue?: T): this;
/** @deprecated since v7, instead use choices or a custom function */
option(flags: string, description: string, regexp: RegExp, defaultValue?: string | boolean | string[]): this;

Example calls are:

.option('-a`, 'no default of coerce')
.option('-b <value>', 'some default', 'xyz')
.option('-c <value>', 'custom processing', parseFloat)
.option('-d <value>, 'custom processing and default', parseFloat, 1.23);

@shadowspawn
Copy link
Collaborator

What is your use case that currently causes a type warning?

@wwwenjie
Copy link
Contributor Author

Thanks for pointing out. That make since. Cuurent I'm using a default value of number. I'm not sure if we need add the number type to default values, i think it depends on the uasge of uesrs.

.option('-d, --deep <length>', 'length of cards for selecting', 5)

warning: Argument type number is not assignable to parameter type string | boolean | undefined   Type number is not assignable to type string | boolean     Type number is not assignable to type boolean

@wwwenjie
Copy link
Contributor Author

maybe use custom processing is a better way...

.option('-d, --deep <length>', 'length of cards for selecting', parseInt, 5)

@shadowspawn
Copy link
Collaborator

Yes. (I was typing up a longer answer when you suggested it yourself! 😄 )

Without custom processing, you would need to deal with mixed types in your code.

cards # number 5 from default
cards --deep=5 # string '5' from CLI

With the custom processing function, Commander can warn you if the default value does not match the return type of the coerce function.

Note: You can not use parseInt directly because it takes 2 arguments and Commander passes two arguments to allow "reduce" type functions. You need to wrap the call and pass parseInt just the value. There is an example in the README.

When you digest all this, was the warning actually useful in this case?

@wwwenjie
Copy link
Contributor Author

Yes! The warning is useful. Sorry I just glanced at the readme and missed its best practices. Thank you so much 😄

@wwwenjie wwwenjie closed this May 13, 2022
@shadowspawn
Copy link
Collaborator

Good. Thanks for confirming. 😄

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