| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
IMO there should be a flag, something like allowNegativeArguments in the options (My name probably needs some work). Someone may be be parsing a --no- prefixed argument for the general CLI args. Tip I am not a core collaborator, and this is only a suggestion. |
Sorry, something went wrong.
|
Turning this on by default would be a breaking change. Might make more sense to make it opt-in per option, as in { 'foo': { type: 'boolean', allowNegation: true } }. Also I'm not sure it makes sense to support this for multiple: true boolean options. That's intended to be for stuff like -vvv, whereas this is for single flags which can be on or off.
That's definitely wrong. --no-foo means foo is false, regardless of what foo defaults to. |
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think it should be set to the opposite of the default value, but to false.
Sorry, something went wrong.
Thanks for the advise!, I believe a flag like allowNegativeArguments or something is better, which will not bring a breaking change. And I think the flag should in the config just like strict or allowPositionals instead of in every options, this will be friendly towards the general CLI args like process.argv. |
Sorry, something went wrong.
Thanks for the reminder! I have misunderstanding it before and thought --no- prefix should just opposite the default value.
I believe add a new flag in the config to determine whether allow bidirectional arguments will be better for avoiding the breaking change.
I think that the --no- prefix and multiple: true are independent of each other. For type: 'boolean' option foo, since ['--foo', '--foo'] is allowed, ['--foo', '--no-foo'] should also be allowed. |
Sorry, something went wrong.
Thanks for the reminder. I have misunderstanding it before and thought --no- prefix should just opposite the default value. |
Sorry, something went wrong.
There was a problem hiding this comment.
| * `allowNegative` {boolean} Whether allow negative options. | |
| * `allowNegative` {boolean} Whether allow negative options prefixed with `--no-`. |
Sorry, something went wrong.
There was a problem hiding this comment.
"Negative options" is a bit unclear. Maybe something more explicit?
| * `allowNegative` {boolean} Whether allow negative options. | |
| * `allowNegative` {boolean} If `true`, allows explicitly setting boolean options to `false` by prefixing the option name with `--no-`. |
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved. Thanks
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
|
If landing this, it would be good to also update the docs which use this precise case as an example of the tokens array:
This example should either be replaced or should at least mention that you can do this automatically with the allowNegative option. Maybe something like:
|
Sorry, something went wrong.
|
I was thinking about that too. The example in the docs support --no-* for a "string" option too, so is doing something different and not completely obsoleted by allowNegative. Say:
|
Sorry, something went wrong.
There was a problem hiding this comment.
The test in checkOptionUsage is against 'boolean' so I think probably clearer and more consistent to test against 'boolean' here?
| if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') !== 'string') { | |
| if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') === 'boolean') { |
Sorry, something went wrong.
There was a problem hiding this comment.
The test in checkOptionUsage is against 'boolean' so I think probably clearer and more consistent to test against 'boolean' here?
here I have considered if options come from CLI, just like node index.js --no-foo, in this case, (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') will be undefined, so it also should be added when using if (... === 'boolean'), which is better for consistent testing.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, so strict:false and allowNegation means --no-foo returns foo:false. Right, I missed that and agree your code is correct and my suggestion wrong. 👍
This is very edge case, but what about strict:false and --no-foo=x? I think to be consistent with general behaviour of --x=y in strict:false should probably return no-foo:x?
The high level intention is when end-user does something unexpected, preserve information and leave it to author to sort out. (See for example the code a few lines down that only sets newValue totrue only if there is not an option value supplied.)
(Edit: added suggestion to cover --no-foo=x in separate comment.)
Sorry, something went wrong.
|
For interest, I was wondering about a short option for negation, and current state of PR does allow this: const parsedArgs = parseArgs({
allowNegative: true,
options: {
boolean: { type: 'boolean', short: 'b' },
'no-boolean': { type: 'boolean', short: 'B' },
}
});
console.log(parsedArgs);
% node index.js
{ values: [Object: null prototype] {}, positionals: [] }
% node index.js --no-boolean
{
values: [Object: null prototype] { boolean: false },
positionals: []
}
% node index.js -B
{
values: [Object: null prototype] { boolean: false },
positionals: []
}
|
Sorry, something went wrong.
I'm curious if the example of --no-logfile in the document might cause confusion, because it actually change the type of option logfile. So I believe this should be update after introducing allowNegative, or allowNegative also should support options which has type: "string", for example, if --no-logfile specified, option logfile ({type: 'string'}) should have value false? |
Sorry, something went wrong.
I have referred the usage of negative options in configure.py, seems there no such usage like a short option for negation, I will update this case |
Sorry, something went wrong.
To be clear, I think the current PR behaviour is fine. I tried that configuration because it is natural in Commander, which has separate options for the positive and negative configurations, so obvious that can have a short option for the negative. I wanted to see if I could do it in parseArgs and it worked. |
Sorry, something went wrong.
I think it is a simpler and more predictable behaviour for allowNegation to only work with boolean options since it is global configuration and not per-option. Otherwise, all options including strings would possibly return false. As for whether we need a new example... |
Sorry, something went wrong.
|
There are some other example uses for tokens on the parseArg repo. Perhaps blocking repeated options? |
Sorry, something went wrong.
There was a problem hiding this comment.
Suggest removing first option so can tell that last-one-wins.
| const args = ['--alpha', '--no-alpha', '--alpha']; | |
| const args = ['--no-alpha', '--alpha']; |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
Sorry, something went wrong.
PR-URL: nodejs#53107 Refs: nodejs#53095 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Notable changes: deps,lib,src: * (SEMVER-MINOR) add experimental web storage (Colin Ihrig) #52435 doc: * move `node --run` stability to rc (Yagiz Nizipli) #53433 * mark WebSocket as stable (Matthew Aitken) #53352 * mark --heap-prof and related flags stable (Joyee Cheung) #53343 * mark --cpu-prof and related flags stable (Joyee Cheung) #53343 * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 lib: * (SEMVER-MINOR) add diagnostics_channel events to module loading (RafaelGSS) #44340 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53583
Sorry, something went wrong.
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 doc: * doc-only deprecate OpenSSL engine-based APIs (Richard Lau) #53329 * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 inspector: * fix disable async hooks on Debugger.setAsyncCallStackDepth (Joyee Cheung) #53473 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 util: * (SEMVER-MINOR) support `--no-` for argument with boolean type for parseArgs (Zhenwei Jin) #53107 PR-URL: #53945
| Back | FazBrowse Home | New Git URL |
This PR tries to support negative options like the format --no-foo for parseArgs by adding a flag allowNegative in the
config of parseArgs. It works for general CLI flag and options passed to parseArgs .
By default, allowNegative is false in order to bring a breaking change.
Refs: #53095