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

feat: add default option parameter #142

Merged
merged 12 commits into from
Oct 8, 2022
Merged

feat: add default option parameter #142

merged 12 commits into from
Oct 8, 2022

Conversation

Eomm
Copy link
Collaborator

@Eomm Eomm commented Aug 13, 2022

The first implementation of #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
Copy link
Collaborator

bakkot commented Aug 13, 2022

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
Copy link
Member

ljharb commented Aug 13, 2022

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
Copy link
Collaborator

bakkot commented Aug 13, 2022

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
Copy link
Collaborator Author

Eomm commented Aug 13, 2022

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
Copy link
Collaborator

bakkot commented Aug 13, 2022

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
Copy link
Collaborator

shadowspawn commented Aug 13, 2022

(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
Copy link
Collaborator

shadowspawn commented Aug 13, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

Eomm commented Aug 14, 2022

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:

  • support array input when multiple = true
  • do not add into the tokens array the default value.
  • check how the user may know if the value is a default one

👍🏽

@aaronccasanova
Copy link
Collaborator

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).

README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
utils.js Outdated Show resolved Hide resolved
utils.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

@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?

@shadowspawn

This comment was marked as outdated.

test/default-values.js Outdated Show resolved Hide resolved
test/default-values.js Outdated Show resolved Hide resolved
test/default-values.js Outdated Show resolved Hide resolved
@simonplend
Copy link

@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!

@Eomm Eomm changed the title feat: add defaultValue option parameter feat: add default option parameter Sep 9, 2022
@shadowspawn
Copy link
Collaborator

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 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!)

@Eomm
Copy link
Collaborator Author

Eomm commented Sep 10, 2022

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
Copy link
Collaborator Author

Eomm commented Oct 7, 2022

Merged into Node core nodejs/node#44631

I have updated this PR accordingly

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

6 participants