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

util: extend parseArgs to validate relationships between options #46657

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SRHerzog
Copy link
Contributor

Added 'conflicts', 'requires' and 'requiresOne' to parseArgs config.

Fixes: #45062

Added 'conflicts', 'requires' and 'requiresOne' to parseArgs config.

Fixes: nodejs#45062
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Feb 14, 2023
@bakkot
Copy link

bakkot commented Feb 14, 2023

Personally I really don't think it makes sense to handle this kind of argument validation within util.parseArgs. It's relatively easy to do yourself, and is unrelated to the actual parsing: none of these things affect the result you get from the parse, just whether the result is considered "valid". That means it's easy to build this kind of validation yourself, on top of the results from parseArgs, the same way you already need to build stuff like "this should be a number" or "this only accepts the following values".

Also, requiresOne is pretty obscure - as far as I'm aware neither commander nor yargs supports requiresOne. (Nor do parsers in other languages, like Python's argparse.)

If you are going to do this, a few things to think about:

  • How do you want to handle defaults? Yargs treats defaults as causing conflicts, Commander does not. Note that with util.parseArgs you have to use the tokens array to check if an option was provided explicitly or via a default, so defaults work very much like specifying an option yourself and it might be weird if they have different behavior with regards to conflicts. (see also conflicts problem yargs/yargs#1910)
  • Having an option which is present in both requiresOne and requires is pointless and should maybe be an error.
  • Since the conflicts relationship is bidirectional, you shouldn't be able to specify that a requires b and also that b conflicts with a. You have a check which forbids "a requires b and also a conflicts with b", but not the reverse.
  • In strict: true mode, it does not make sense to list options which don't exist in any of these new fields. Arguably it doesn't make sense in non-strict mode either.
  • There are more complex relationships about which options can/must be used together which this is unable to express. Some people will want those. If we're going to allow you to express a subset of relationships, maybe it's better to go all the way and allow you to specify arbitrary propositions about which arguments are specified together. This currently allows kind of a weird subset of propositions - "x implies y" and "x implies (y or z)" and "not (x and y)", but not "(x and y) implies z" or "x implies ((y and z) or w)", etc.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my #44564 (comment):

My main concern here is adding complexity both to the API surface and to the implementation while still not providing a complete solution. The goal has not been to provide a full-fledged and feature-complete argument parser. In this particular case, even with the three newly proposed options (requires, requiresOne, incompatible) not all sets of subsets can be described, so it is not a complete solution to describing relations between the existence of options, while significantly increasing complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util: Extended parseArgs validation settings
4 participants