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

Add string[] to options defaultValue type #1721

Merged
merged 1 commit into from May 9, 2022

Conversation

everett1992
Copy link
Contributor

@everett1992 everett1992 commented Apr 20, 2022

Pull Request

Problem

The default value for a variadic option should be an array but the type requires a boolean or string.
If you pass a string as the default value to a variadic option the default value will be a string and you'll have to handle string or string[] when using the options. If you pass an array you only have to handle arrays, and you can pass a multi-valued default.

Solution

I added string[] to option's defaultValue.

ChangeLog

Add string[] to options defaultValue type.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 20, 2022

The problem is that the .option() method is overloaded. By using 'unknown for the first overload, it will match any argument and mask the next two overloads.

My recollection is in theory the default value can be anything, but in practice it is usually a string or boolean, or the type returned by the coerce function. However, we might have added variadic after that work, and maybe adding string[] does make sense. (I'll check the timing of those features.)

option(flags: string, description?: string, defaultValue?: unknown): this;
option<T>(flags: string, description: string, fn: (value: string, previous: T) => T, defaultValue?: T): this;
option(flags: string, description: string, regexp: RegExp, defaultValue?: unknown): this;

@shadowspawn
Copy link
Collaborator

option and requiredOption's defaultValue parameter was `string |
boolean` but the default for a variadic option should be an array.

```
program.option('--var <args...>', 'variadic arguments', ['1'])
program.parse([])
> { var: ['1'] }

program.parse(['--var', '1', '2'])
> { var: ['1', '2'] }
```

If you use a string default you have to handle opts that are strings
```
// with a string arg the default value is a string.
program.option('--var <args...>', 'variadic arguments', '1')
program.parse([])
> { var: '1' }

program.parse(['--var', '1', '2'])
> { var: ['1', '2'] }
```

`unknown` matches the jsdoc comment and the typings for argument(name:
string, description?: string, defaultValue?: unknown): this` but
conflicts with other `option` overloads.

commander will pass thru any defaultValue to parse opts so `any` or
`unknown` are good choices, but `string | boolean | string[]` reflects
the values that commander will return when it parses arguments without a
coerce function.
@everett1992
Copy link
Contributor Author

that makes sense, I didn't consider the overlaps between override functions. I updated the PR to add string[].

@everett1992 everett1992 changed the title Change defaultValue type to unknown Add string[] to options defaultValue type Apr 20, 2022
@shadowspawn
Copy link
Collaborator

For historical interest, added variadic after the tighter typing was introduced:

@shadowspawn
Copy link
Collaborator

It might be next week before I have time to look into the constraints, but I think string[] is probably what we want.

@shadowspawn shadowspawn self-assigned this Apr 21, 2022
@shadowspawn shadowspawn removed their assignment Apr 30, 2022
@shadowspawn
Copy link
Collaborator

shadowspawn commented Apr 30, 2022

Some extra information for future readers. The default value can technically be any type, but in practice is almost always:

  • string for option with value
  • boolean for boolean option
  • array of strings for variadic option (added in this PR)
  • or same as type returned by custom option processing function (coercion function)

If we use any for the type for default argument then get no type checking of the overloaded calling signatures, so we specify just the expected types to provide help for the normal cases.

If needed, the author can specify some other type without using an explicit coercion by using Option.default():

program.addOption(new Option('-e, --example <value>').default({ very: 'tricky' });

@shadowspawn shadowspawn merged commit 1b492d9 into tj:develop May 9, 2022
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label May 9, 2022
@shadowspawn
Copy link
Collaborator

Included in Commander v9.3.0. Thanks @everett1992

@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label May 29, 2022
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

3 participants