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: Extended parseArgs validation settings #45062

Closed
jhmaster2000 opened this issue Oct 19, 2022 · 11 comments · May be fixed by #46657
Closed

util: Extended parseArgs validation settings #45062

jhmaster2000 opened this issue Oct 19, 2022 · 11 comments · May be fixed by #46657
Labels
feature request Issues that request new features to be added to Node.js. stale util Issues and PRs related to the built-in util module.

Comments

@jhmaster2000
Copy link

jhmaster2000 commented Oct 19, 2022

On issue #44564, a required setting was proposed, but frowned upon due to the ideal optional nature of commandline flags and options, in that same linked issue I have made a counter proposal (#44564 (comment)) about incompatible and requires fields instead? This is a dedicated followup issue to that proposal for further discussion.

The proposal

The addition of 3 new fields to parseArgs' options objects: incompatible, requires and requiresOne.

While unconditionally required options are rare and frowned upon, dependent and incompatible options are far more common. These fields would be of type string[], containing names of other options in the current parseArgs object:

parseArgs({
  args,
  options: {
    owner: {
      type: 'string',
      requires: ['repo'],
      incompatible: ['org'],
    },
    org: {
      type: 'string',
      requires: ['repo'],
      // no need to specify incompatible: ['owner']
      // it should work both ways on either one
      // but it can also be explicitly on both if desired
    },
    repo: {
      type: 'string',
      // details on requiresOne further down
      requiresOne: ['owner', 'org'],
    },
    url: {
      type: 'string',
      incompatible: ['repo', 'org', 'owner'],
    }
  }
});

The above code would result in:

# Allowed
foo --url=https://gh.com/user/proj
foo --owner=user --repo=proj
foo --org=nodejs --repo=node
foo

# Not allowed
foo --url=https://... --owner=user
foo --org=nodejs --url=https://...
foo --repo=proj --owner=user --url=https://...
foo --owner=user --org=nodejs --repo=proj
foo --repo=node
foo --owner=user
foo --org=nodejs
  • All of these new fields would default to an empty array []. If any of them specify either their own option or a non-existent option it should throw a TypeError.
  • requires is directional, if --a requires --b but --b doesn't require --a, foo --b is valid but foo --a is not.
  • requiresOne is a variation of requires where the array is treated as a list of ORs instead of ANDs.
  • requires and requiresOne can be used together without issue, but naturally overlapping options between the two will have higher precedence on requires.
  • incompatible is bidirectional, if --a declared itself incompatible with --b, then --b is also implicitly incompatible with --a. Explicitly marking both incompatible with each other is fine and does nothing extra.
  • If a conflict between the fields requires* and incompatible of two options or one by itself happens, it should throw a TypeError.

Notes

  • The names of the 3 fields are not final and could be discussed further, some possible alternatives I have though of:
    • requiresAll might be considered more consistent with requiresOne than requires.
    • Finding a way to somehow merge requires and requiresOne into a single option might be a possibility too but I couldn't think of a good way.
    • incompatible -> incompatibleWith, slightly longer but more self-explanatory.
    • incompatible -> conflicts, compatibility with Commander and Yargs (util: Extended parseArgs validation settings #45062 (comment))
@jhmaster2000
Copy link
Author

Replying to these here to move the discussion.
Previous discussion for reference: #44564 (comment) #44564 (comment)

In response to @tniessen #44564 (comment):

I don't see this as added complexity to the API surface since as previously mentioned all of these are 100% optional and do not need to be used, the current simple API would remain usable with no change. As for added complexity to the implementation, yes that's true, but I also see it as an argument in favor of adding it once more, given that if it isn't done by parseArgs, userland code will need to do said complex heavy lifting on its own, which can be prone to mistakes and results in hundreds of different implementations each with their quirks, and having users not need to do such heavy lifting themselves is precisely the goal of utility builtins.

In response to @shadowspawn #44564 (comment):

It is actually a little funny how there is seemingly a paradox here, since as mentioned, there have been arguments in the case of the default values for parseArgs, that it was "too simple" of an addition to be worthwhile to add to the utility since userland code could easily implement such functionality on its own, and now the problem is "too complex", which contradicts the previous, something to think about I suppose...

@shadowspawn
Copy link
Member

(There are so many possible additions to argument parsing. I am checking whether proposed changes are a clean addition and widely desired, whether simple or complex to add. No paradox from my point of view, but yes gentle pushback on additions.)

@shadowspawn
Copy link
Member

shadowspawn commented Oct 19, 2022

Commander and Yargs both use conflicts() as the name for the suggested incompatible.

(The naming is not independent. Commander only recently added conflicts() after 10 years without any such routines, and I looked at the Yargs naming.)

@shadowspawn
Copy link
Member

An implementation detail is whether such routines consider just the options from the command-line (or supplied args), or consider the returned value of the option. The difference is the default values.

From the wording referencing --a I am guessing you are thinking of the command-line option and ignoring the default value.

The reason for considering the default value is the author might stick in an environment variable as the default value. This means the option default value may be from the end-user.

@jhmaster2000
Copy link
Author

Commander and Yargs both use conflicts() as the name for the suggested incompatible.

(The naming is not independent. Commander only recently added conflicts() after 10 years without any such routines, and I looked at the Yargs naming.)

conflicts sounds fine to me aswell, I'd be happy with either, I'll add it as a possible alternative 👍

An implementation detail is whether such routines consider just the options from the command-line (or supplied args), or consider the returned value of the option. The difference is the default values.

From the wording referencing --a I am guessing you are thinking of the command-line option and ignoring the default value.

The reason for considering the default value is the author might stick in an environment variable as the default value. This means the option default value may be from the end-user.

I did overlook defining this behavior in the main post, but I think requires* and default should simply be incompatible with each other, thinking about it from a JS/TS functions perspective, when you give an argument a default value, it becomes entirely optional and thus cannot be required, so an option with a default would mean explicitly optional at all times and trying to require it should error.

Conditionally doing it based on the truthiness of the default value concerns me because there is no guarantee an intended default value will always be a truthy JS value, namely false or an empty string.

It could maybe be made so that the errors thrown provide sufficient parseable information so that if desired userland code can wrap the parseArgs call in a try catch and implement additional custom behavior for specific errors such as checking environment variable, database or cached data (etc) availability, since this really is one thing that just has too wide range of possibilities to make a good unclogged API for in my opinion, we would probably be talking about stuff like adding callbacks into this to support that in the parseArgs API itself.

@VoltrexKeyva VoltrexKeyva added the util Issues and PRs related to the built-in util module. label Oct 19, 2022
@tniessen tniessen added the feature request Issues that request new features to be added to Node.js. label Oct 20, 2022
@tniessen
Copy link
Member

It is actually a little funny how there is seemingly a paradox here, since as mentioned, there have been arguments in the case of the default values for parseArgs, that it was "too simple" of an addition to be worthwhile to add to the utility since userland code could easily implement such functionality on its own, and now the problem is "too complex", which contradicts the previous, something to think about I suppose...

It's not really a contradiction; complexity can be orthogonal to usefulness, and apparently, enough people saw the usefulness in default values (even though I personally might have preferred not to add default values to parseArgs). This proposal, on the other hand, appears much more complex and, according to #45062 (comment), commander did not have this feature for an entire decade, so it might not be as desired.

(Also, nobody said that this feature would be "too complex" to do in userland.)

@jhmaster2000
Copy link
Author

In my opinion if people continue importing commander or other third party CLI libraries for these kinds of features that parseArgs might not have, it kind of takes away the value of having parseArgs built-in, which would have been to make argument handling easy without the need of third party libraries. Sure some basic CLI programs may have everything they need from parseArgs alone, but a standard library should try to account for all (or a majority of) usecases.

@bakkot
Copy link

bakkot commented Dec 2, 2022

Sure some basic CLI programs may have everything they need from parseArgs alone, but a standard library should try to account for all (or a majority of) usecases.

I disagree. It's much more important that the standard library be easy to use and reliable, which is directly in tension with having a bunch of options. It's fine if less-common use cases require you to implement a little logic yourself, even if that means some people will reach for a third-party library instead.

@SRHerzog
Copy link
Contributor

Here's a draft PR to help move the discussion forward. I don't see these options adding any instability or inconvenience for users who don't need them, other than an extra paragraph to skip over in the docs.

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Aug 14, 2023
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2023
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Added 'conflicts', 'requires' and 'requiresOne' to parseArgs config.

Fixes: nodejs/node#45062
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants