GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — util: support negative options for `parseArgs` by kylo5aby · Pull Request #53107 · nodejs/node Conversation
|
|
IMO there should be a flag, something like Tip I am not a core collaborator, and this is only a suggestion. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Turning this on by default would be a breaking change. Might make more sense to make it opt-in per option, as in Also I'm not sure it makes sense to support this for
That's definitely wrong. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be set to the opposite of the default value, but to false.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Thanks for the advise!, I believe a flag like |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Thanks for the reminder! I have misunderstanding it before and thought
I believe add a new flag in the
I think that the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 an error while loading. Please reload this page.
--no- prefix for argument with boolean type for parseArgsparseArgs
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| * `allowNegative` {boolean} Whether allow negative options. | |
| * `allowNegative` {boolean} Whether allow negative options prefixed with `--no-`. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
"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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Resolved. Thanks
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I was thinking about that too. The example in the docs support
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The test in
checkOptionUsageis 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 an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
For interest, I was wondering about a short option for negation, and current state of PR does allow this: % 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I'm curious if the example of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I have referred the usage of negative options in |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I think it is a simpler and more predictable behaviour for As for whether we need a new example... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There are some other example uses for tokens on the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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 an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 4a72b2f |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: #53107 Refs: #53095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#53107 Refs: nodejs#53095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
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.
There was an error while loading. Please reload this page.
PR-URL: #53107 Refs: #53095 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
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
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.