util: support negative options for `parseArgs` #53107

kylo5aby · 2024-05-22T18:45:17Z

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

RedYetiDev · 2024-05-22T19:08:57Z

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.

bakkot · 2024-05-22T21:49:52Z

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.

If the argument is passed with the --no- prefix, the value of the argument will be set to the opposite of its default value.

That's definitely wrong. --no-foo means foo is false, regardless of what foo defaults to.

mcollina

I don't think it should be set to the opposite of the default value, but to false.

kylo5aby · 2024-05-23T08:15:01Z

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.

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.

kylo5aby · 2024-05-23T08:31:41Z

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.

If the argument is passed with the --no- prefix, the value of the argument will be set to the opposite of its default value.

That's definitely wrong. --no-foo means foo is false, regardless of what foo defaults to.

That's definitely wrong. --no-foo means foo is false, regardless of what foo defaults to.

Thanks for the reminder! I have misunderstanding it before and thought --no- prefix should just opposite the default value.

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 } }.

I believe add a new flag in the config to determine whether allow bidirectional arguments will be better for avoiding the breaking change.

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.

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.

kylo5aby · 2024-05-23T08:36:51Z

I don't think it should be set to the opposite of the default value, but to false.

Thanks for the reminder. I have misunderstanding it before and thought --no- prefix should just opposite the default value.
In order to avoiding the breaking change, add a new flag in config for parseArgs to control whether allow bidirectional arguments, what do you think?

MoLow · 2024-05-24T05:25:09Z

doc/api/util.md

@@ -1429,6 +1429,8 @@ changes:
  * `allowPositionals` {boolean} Whether this command accepts positional
    arguments.
    **Default:** `false` if `strict` is `true`, otherwise `true`.
+  * `allowNegative` {boolean} Whether allow negative options.


Suggested change

* `allowNegative` {boolean} Whether allow negative options.

* `allowNegative` {boolean} Whether allow negative options prefixed with `--no-`.

"Negative options" is a bit unclear. Maybe something more explicit?

Suggested change

* `allowNegative` {boolean} Whether allow negative options.

* `allowNegative` {boolean} If `true`, allows explicitly setting boolean options to `false` by prefixing the option name with `--no-`.

Resolved. Thanks

mcollina

lgtm

nodejs-github-bot · 2024-05-24T06:13:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59381/

bakkot · 2024-05-24T06:18:27Z

If landing this, it would be good to also update the docs which use this precise case as an example of the tokens array:

For example to use the returned tokens to add support for a negated option like --no-color, the tokens can be reprocessed to change the value stored for the negated option.

This example should either be replaced or should at least mention that you can do this automatically with the allowNegative option. Maybe something like:

For example, you can replicate the behavior of the allowNegative option (which adds support for a negated option like --no-color) by reprocessing the tokens array to change the value stored for the negated option.

shadowspawn · 2024-05-24T06:21:53Z

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:

For example to use the returned tokens to add support for a negated option like --no-color independent of the option type, the tokens can be reprocessed to change the value stored for the negated option.

shadowspawn · 2024-05-24T06:50:31Z

lib/internal/util/parse_args/parse_args.js

+  if (allowNegative && StringPrototypeStartsWith(longOption, 'no-')) {
+    // Boolean option negation: --no-foo
+    const longOptionWithoutPrefixNo = StringPrototypeSlice(longOption, 3);
+    if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') !== 'string') {


The test in checkOptionUsage is against 'boolean' so I think probably clearer and more consistent to test against 'boolean' here?

Suggested change

if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') !== 'string') {

if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') === 'boolean') {

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.

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.)

shadowspawn · 2024-05-24T07:08:28Z

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: []
}

kylo5aby · 2024-05-24T07:54:18Z

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:

For example to use the returned tokens to add support for a negated option like --no-color independent of the option type, the tokens can be reprocessed to change the value stored for the negated option.

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?

kylo5aby · 2024-05-24T08:58:39Z

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: []
}

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

shadowspawn · 2024-05-24T09:18:58Z

For interest, I was wondering about a short option for negation, and current state of PR does allow this:

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.

shadowspawn · 2024-05-24T09:31:58Z

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?

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...

shadowspawn · 2024-05-24T09:37:45Z

There are some other example uses for tokens on the parseArg repo. Perhaps blocking repeated options?

https://github.com/pkgjs/parseargs/blob/main/examples/no-repeated-options.js

shadowspawn · 2024-05-26T03:35:14Z

test/parallel/test-parse-args.mjs

+});
+
+test('allow negative options and passed multiple arguments', () => {
+  const args = ['--alpha', '--no-alpha', '--alpha'];


Suggest removing first option so can tell that last-one-wins.

Suggested change

const args = ['--alpha', '--no-alpha', '--alpha'];

const args = ['--no-alpha', '--alpha'];

nodejs-github-bot · 2024-05-27T14:34:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59455/

nodejs-github-bot · 2024-06-19T10:05:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59880/

nodejs-github-bot · 2024-06-19T10:17:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59881/

nodejs-github-bot · 2024-06-19T12:46:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59885/

nodejs-github-bot · 2024-06-19T13:55:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59886/

nodejs-github-bot · 2024-06-19T15:36:19Z

Landed in 4a72b2f

PR-URL: #53107 Refs: #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>

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

ljharb · 2024-07-15T22:09:04Z

DefinitelyTyped/DefinitelyTyped#70056

PR-URL: #53107 Refs: #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: 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: TODO

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

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels May 22, 2024

mcollina requested changes May 23, 2024

View reviewed changes

kylo5aby force-pushed the node-util branch from 2b5871d to 0c957ea Compare May 24, 2024 03:55

kylo5aby changed the title ~~util: support --no- prefix for argument with boolean type for parseArgs~~ util: support negative options for parseArgs May 24, 2024

MoLow reviewed May 24, 2024

View reviewed changes

MoLow approved these changes May 24, 2024

View reviewed changes

mcollina approved these changes May 24, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2024

kylo5aby force-pushed the node-util branch from 0c957ea to b2c496d Compare May 24, 2024 06:08

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 24, 2024

shadowspawn reviewed May 24, 2024

View reviewed changes

jasnell approved these changes May 25, 2024

View reviewed changes

atlowChemi approved these changes May 25, 2024

View reviewed changes

atlowChemi added the semver-minor PRs that contain new features and should be released in the next minor version. label May 25, 2024

shadowspawn reviewed May 26, 2024

View reviewed changes

legendecas added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 19, 2024

legendecas approved these changes Jun 19, 2024

View reviewed changes

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2024

nodejs-github-bot merged commit 4a72b2f into nodejs:main Jun 19, 2024
59 checks passed

targos mentioned this pull request Jun 25, 2024

v22.4.0 release proposal #53583

Merged

Karliz24 mentioned this pull request Jul 9, 2024

## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16

Open

bakkot mentioned this pull request Jul 16, 2024

parseArgs with type: "boolean" and default: true does not allow a false value #53095

Closed

marco-ippolito mentioned this pull request Jul 19, 2024

v20.16.0 proposal #53945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: support negative options for `parseArgs` #53107

util: support negative options for `parseArgs` #53107

kylo5aby commented May 22, 2024 •

edited

Loading

RedYetiDev commented May 22, 2024 •

edited

Loading

bakkot commented May 22, 2024

mcollina left a comment

kylo5aby commented May 23, 2024 •

edited

Loading

kylo5aby commented May 23, 2024

kylo5aby commented May 23, 2024

MoLow May 24, 2024

bakkot May 24, 2024

kylo5aby May 24, 2024

mcollina left a comment

nodejs-github-bot commented May 24, 2024

bakkot commented May 24, 2024

shadowspawn commented May 24, 2024 •

edited

Loading

shadowspawn May 24, 2024

kylo5aby May 24, 2024

shadowspawn May 24, 2024 •

edited

Loading

This comment was marked as duplicate.

shadowspawn commented May 24, 2024 •

edited

Loading

kylo5aby commented May 24, 2024 •

edited

Loading

kylo5aby commented May 24, 2024

shadowspawn commented May 24, 2024 •

edited

Loading

shadowspawn commented May 24, 2024

shadowspawn commented May 24, 2024

shadowspawn May 26, 2024

shadowspawn May 27, 2024

nodejs-github-bot commented May 27, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

ljharb commented Jul 15, 2024

	* `allowNegative` {boolean} Whether allow negative options.
	* `allowNegative` {boolean} Whether allow negative options prefixed with `--no-`.

	* `allowNegative` {boolean} Whether allow negative options.
	* `allowNegative` {boolean} If `true`, allows explicitly setting boolean options to `false` by prefixing the option name with `--no-`.

	if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') !== 'string') {
	if (optionsGetOwn(options, longOptionWithoutPrefixNo, 'type') === 'boolean') {

	const args = ['--alpha', '--no-alpha', '--alpha'];
	const args = ['--no-alpha', '--alpha'];

util: support negative options for parseArgs #53107

util: support negative options for parseArgs #53107

Conversation

kylo5aby commented May 22, 2024 • edited Loading

RedYetiDev commented May 22, 2024 • edited Loading

bakkot commented May 22, 2024

mcollina left a comment

Choose a reason for hiding this comment

kylo5aby commented May 23, 2024 • edited Loading

kylo5aby commented May 23, 2024

kylo5aby commented May 23, 2024

MoLow May 24, 2024

Choose a reason for hiding this comment

bakkot May 24, 2024

Choose a reason for hiding this comment

kylo5aby May 24, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 24, 2024

bakkot commented May 24, 2024

shadowspawn commented May 24, 2024 • edited Loading

shadowspawn May 24, 2024

Choose a reason for hiding this comment

kylo5aby May 24, 2024

Choose a reason for hiding this comment

shadowspawn May 24, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as duplicate.

shadowspawn commented May 24, 2024 • edited Loading

kylo5aby commented May 24, 2024 • edited Loading

kylo5aby commented May 24, 2024

shadowspawn commented May 24, 2024 • edited Loading

shadowspawn commented May 24, 2024

shadowspawn commented May 24, 2024

shadowspawn May 26, 2024

Choose a reason for hiding this comment

shadowspawn May 27, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented May 27, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

nodejs-github-bot commented Jun 19, 2024

ljharb commented Jul 15, 2024

util: support negative options for `parseArgs` #53107

util: support negative options for `parseArgs` #53107

kylo5aby commented May 22, 2024 •

edited

Loading

RedYetiDev commented May 22, 2024 •

edited

Loading

kylo5aby commented May 23, 2024 •

edited

Loading

shadowspawn commented May 24, 2024 •

edited

Loading

shadowspawn May 24, 2024 •

edited

Loading

shadowspawn commented May 24, 2024 •

edited

Loading

kylo5aby commented May 24, 2024 •

edited

Loading

shadowspawn commented May 24, 2024 •

edited

Loading