-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: support more config options for parseArgs #53434
Conversation
4b37afe
to
6108de4
Compare
I'm all ears to better name options than |
Maybe I'm misunderstanding the request here, but would the following work:
|
IMO, "optional" doesn't explain what's this is doing. This flag is meant for if the flag is supplied, but does not contain a value. |
Ah, I was indeed misunderstanding the request. Carry on 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in #53427 (comment), parseArgs()
is supposed to be a minimal implementation. It is not meant for anything beyond the most basic use cases.
More importantly, I don't see how this PR addresses #53427. With this API, how is the application supposed to distinguish between the absence of the option (in which case the option gets the default value) and the presence of the option without a value (in which case the option gets the default value)? In what scenario is useDefaultWhenUndefined
useful?
Before this API, when an argument was supplied without a value ( An example use case is when you want the user to have the option to enable/customize an option. ./my_script.js --color # Green color
./my_script.js --color=yellow # Yellow color
./my_script.js # Green color |
6108de4
to
f9d1ef5
Compare
My latest commit undid itself 🤔 (Fixed) |
@RedYetiDev Again, I do not think that's what the feature request is describing. The first and third case in your example should not have the same result, and they do not in the case of |
Makes sense, I can adjust the specifics later today |
f9d1ef5
to
7cf1c0f
Compare
|
While it wasn't mentioned, I felt that the addition of a placeholder also warranted the addition of a required, as a user may want to use the placeholder on a required flag. |
The comments on the PR requesting this feature appear to have reached a consensus that this implemtation should remain minimal. |
I agree that the feature should be minimal, but I also think that the main problem with the approach in this PR is that it makes the parsing ambiguous from a user's point of view. |
Fixes #53427
This pull request introduces the ability for an argument in
parseArgs
to have therequired
andplaceholder
options.required
means that an argument must be supplied.placeholder
is the value for an argument when it is supplied, but no value is supplied with it.