-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add default
option parameter
#142
Conversation
Thanks for the PR! Personally I'm not totally sure this is worth adding, vs just expecting users to write
This is definitely not right. The |
It’s very useful to provide the default value by configuration and not just in code, for example to include it in help output, or to ensure the default value is normalized/validated just like user-provided values. |
Well, sure, but that doesn't mean it's necessarily worth adding to the parser itself. It's literally one line to implement it yourself: let options = { foo: { type: 'string', default: 'bar' }, ...etc };
let { values } = parseArgs({ options });
Object.keys(options).forEach(k => { values[k] ??= options[k].default }); |
How would you support/implement the default value? |
Pretty much the same way you'd do it in userland: just before returning the parsed values, go through the configured options and set the default value for any configured key which has a default but which does not already have a value arising from the parse itself. (There's details you'd still need to work out - how do you handle |
(Adding to what @bakkot already said.) I see the temptation to put the defaults into the tokens so they are represented. But the (A different thing we might have represented would be "events" that occur in the parsing, and a default would have fitted into that.) |
I agree in part. However, |
I suggest adding a default value which appears in returned |
I think there is somewhat low value in adding support for defaults, but I think it is something people want to do and look for, and a good thing to consider and discuss. Thanks for raising it @Eomm . 😄 |
We can support it by accepting an array
I may try to arrange an example or think a similar output is needed to get the "default-tokens". To recap:
👍🏽 |
Personally would love to see this feature land! While I understand it is a one liner for users to add, we have the underlying utilities in place to apply run time validation and rich TypeScript support. Light suggestion to rename |
f406d8e
to
07d97be
Compare
@bakkot commented that not totally sure this is worth adding: #142 (comment) (and Aaron and I both agree not much code saved). We have had multiple people expressing interest in default support, and for that reason I lean towards including it as something people look for that suits how they wish to configure their options.
Anyone want to make an argument that we shouldn't include defaults? |
This comment was marked as outdated.
This comment was marked as outdated.
94b3b1b
to
662664d
Compare
@Eomm You might want to update the PR title and description to reflect the change in naming from This is a great addition — looking forward to seeing it land! |
default
option parameter
Now that For example #129 (123 comments in this repo during active development) and nodejs/node#43459) (60 comments). @Eomm has addressed all the code issues we have raised here. Are you up for proposing it for Node.js @Eomm ? (Otherwise, I'm willing to give it a go. I have only made one upstream PR myself, so not an expert offer!) |
Yeah! I will do it, it would be fantastic to try to submit a PR to Node.js core! |
ee2bc06
to
b2f6a44
Compare
Merged into Node core nodejs/node#44631 I have updated this PR accordingly |
The first implementation of #82
This PR adds a new
default
parameter.It lets the user specify an option's default value when it is not set by the
args
input.When it is set, the
option
token is added automatically.This pr does not include any process.env logic, but the user may set
default: process.env.WHATEVER