pkgjs / parseargs

Polyfill of `util.parseArgs()`
Apache License 2.0
121 stars 9 forks source link

feat: add `default` option parameter #142

Closed Eomm closed 1 year ago

Eomm commented 2 years ago

The first implementation of https://github.com/pkgjs/parseargs/issues/82

This PR adds a new default parameter.

  const args = [];
  const options = {
    a: { type: 'string', default: 'HELLO' }
  };
  const result = parseArgs({ args, options }); // { values: { a: 'HELLO' } }

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

bakkot commented 2 years ago

Thanks for the PR! Personally I'm not totally sure this is worth adding, vs just expecting users to write values.foo ?? 'default'. but it's good to have something to discuss.

When it is set, the option token is added automatically.

This is definitely not right. The tokens array should represent the input exactly; no more and no less.

ljharb commented 2 years ago

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.

bakkot commented 2 years ago

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 });
Eomm commented 2 years ago

This is definitely not right. The tokens array should represent the input exactly; no more and no less.

How would you support/implement the default value? I think the use case is: "when there is no input for a given option, the application requires the default value"

bakkot commented 2 years ago

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 multiple options, in particular - but the general approach of modifying the values and not the tokens is the part I want to emphasize.)

shadowspawn commented 2 years ago

(Adding to what @bakkot already said.)

I see the temptation to put the defaults into the tokens so they are represented. But the tokens just represent the parsing of the input arguments. Every token has an index property which is the index into the input arguments where the token came from.

(A different thing we might have represented would be "events" that occur in the parsing, and a default would have fitted into that.)

shadowspawn commented 2 years ago

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.

I agree in part. However, parseArgs does not offer help and does not offer normalise/validate.

shadowspawn commented 2 years ago

How would you support/implement the default value?

I suggest adding a default value which appears in returned values is sufficient. I know people sometimes want to know whether the value came from the input args or from the default value. I don't think this is the common case, and can be determined by looking in the tokens.

shadowspawn commented 2 years ago

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 . πŸ˜„

Eomm commented 2 years ago

how do you handle multiple options

We can support it by accepting an array

I don't think this is the common case, and can be determined by looking in the tokens.

I may try to arrange an example or think a similar output is needed to get the "default-tokens".

To recap:

πŸ‘πŸ½

aaronccasanova commented 2 years ago

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 defaultValue to default. We've conventionally used terse names where possible and in the context of an option config object I think default will naturally imply it's the default value (take meow for example).

shadowspawn commented 2 years ago

@bakkot commented that not totally sure this is worth adding: https://github.com/pkgjs/parseargs/pull/142#issuecomment-1214171608 (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?

shadowspawn commented 2 years ago

Light suggestion to rename defaultValue to default. We've conventionally used terse names where possible and in the context of an option config object I think default will naturally imply it's the default value (take meow for example).

I second the suggestion for the shorter name. Someone using defaults is likely to have it appear multiple times in their configuration.

(Commander and Yargs also both have default rather than defaultValue.)

simonplend commented 1 year ago

@Eomm You might want to update the PR title and description to reflect the change in naming from defaultValue to default.

This is a great addition β€” looking forward to seeing it land!

shadowspawn commented 1 year ago

Now that parseArgs is in node we have been landing the changes in Node.js first.

For example #129 (123 comments in this repo during active development) and https://github.com/nodejs/node/pull/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!)

Eomm commented 1 year ago

Are you up for proposing it for Node.js @Eomm ?

Yeah! I will do it, it would be fantastic to try to submit a PR to Node.js core! Thanks for the helpful example PR

Eomm commented 1 year ago

Merged into Node core https://github.com/nodejs/node/pull/44631

I have updated this PR accordingly