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

Truncated, falsy arguments starting with --no #38

Closed
andrewmclagan opened this issue May 31, 2023 · 19 comments · Fixed by #48
Closed

Truncated, falsy arguments starting with --no #38

andrewmclagan opened this issue May 31, 2023 · 19 comments · Fixed by #48
Labels
documentation Improvements or additions to documentation help wanted pull requests welcome

Comments

@andrewmclagan
Copy link

andrewmclagan commented May 31, 2023

$ node example/parse.js --no-foo --yes-foo
{ _: [], foo: false, 'yes-foo': true }

Odd behaviour?

$ node example/parse.js --no --yes
{ _: [], no: true, yes: true }
@andrewmclagan andrewmclagan changed the title Truncated arguments starting with --no Truncated, falsey arguments starting with --no May 31, 2023
@shadowspawn
Copy link
Collaborator

This is an undocumented feature. A common pattern with command-line options is to have a lone option starting with --no-, or a pair of options for positive and negative with --no- in front of the negated option. For example git clone supports:

--[no-]single-branch
--no-tags

If an option is given to Minimist as --no-example, as a convenience the option example is set to false (rather than no-example to true).

% node index.js --no-x
{ _: [], x: false }
% node index.js --x   
{ _: [], x: true }

@ljharb
Copy link
Member

ljharb commented May 31, 2023

Let's document it, it's pretty universal (yargs does it too).

@ljharb ljharb added documentation Improvements or additions to documentation help wanted pull requests welcome labels May 31, 2023
@andrewmclagan
Copy link
Author

Ah good to know. We ended up going with --skip-thing rather than --no-thing as a workaround

@ljharb
Copy link
Member

ljharb commented Jun 1, 2023

Let's keep this open to track updating the docs.

@ljharb ljharb reopened this Jun 1, 2023
@shadowspawn
Copy link
Collaborator

For interest, same issue in a Minimist fork: meszaros-lajos-gyorgy/minimist-lite#10

@JonnyBurger
Copy link

Very surprising indeed. We added a --no-open flag, expecting {['no-open']: true}, but it actually gives {['no-open']: false, open: true}

@ljharb
Copy link
Member

ljharb commented Oct 10, 2023

I’m surprised that’s surprising, but either way we should update our docs to make it clear.

@JonnyBurger
Copy link

Ok, I can explain why it is surprising:

We have several flags, for example: --port, --no-open, --headless.
Then our logic in our code is somewhat like that:

headless = false;
open = true
if (cliFlags.headless) {
	headless = true
}
if (cliFlags['no-open']) {
	open = false
}

Leading to a minor bug, because when reading the README on NPM one didn't learn about this behavior.
But it seems already better documented now on main, so just pushing to NPM could help.

@ljharb ljharb changed the title Truncated, falsey arguments starting with --no Truncated, falsy arguments starting with --no Oct 10, 2023
@ljharb
Copy link
Member

ljharb commented Oct 10, 2023

I will say that it seems reasonable to me to have no-open set to false (but be non-enumerable) AND open set to true, since that's reasonable, consistent, and would guard against that particular bug. @shadowspawn thoughts?

@shadowspawn
Copy link
Collaborator

We added a --no-open flag, expecting {['no-open']: true}, but it actually gives {['no-open']: false, open: true}

Just checking I correctly understand what you saw @JonnyBurger . Did you mean open:false in the "actual" behaviour? I see something similar but different to your description like this:

// note: declaring `no-open` here is doomed, but trying to reproduce description
var argv = require('minimist')(process.argv.slice(2), {
    boolean: ['no-open', 'headless']
});
console.log(argv);
% node index.js          
{ _: [], 'no-open': false, headless: false }
% node index.js --no-open
{ _: [], 'no-open': false, headless: false, open: false }

Basically there is currently only --no-foo support when processing the command-line and there is an implicit expectation that you use foo in the configuration. That makes me think #48 might be a bit thin.

@shadowspawn
Copy link
Collaborator

I will say that it seems reasonable to me to have no-open set to false (but be non-enumerable) AND open set to true, since that's reasonable, consistent, and would guard against that particular bug. @shadowspawn thoughts?

I am not a fan of multiple versions of flags being stored, but it is an interesting idea to avoid surprises at runtime in this case.

What did you mean by "but be non-enumerable" @ljharb ? (That it doesn't show up when parse result logged?)

@ljharb
Copy link
Member

ljharb commented Oct 11, 2023

Yep, what i mean is, if you Get the property directly it'll work, otherwise it'll be pretty hard to know it's there.

@JonnyBurger
Copy link

@shadowspawn Yes, you got it correctly! I am using the boolean array as well and was checking for the no-open flag to be set.

The feature is fine, but not documented and therefore I was surprised.

Also, the other reason I got confused is because I used @types/minimist, and argv["no-open"] is autocompleted while argv.open does not seem to exist, and I didn't bother to test the behavior.

TypeScript users might still get trapped even if the property is non-enumerable.

@shadowspawn
Copy link
Collaborator

Also, the other reason I got confused is because I used @types/minimist, and argv["no-open"] is autocompleted while argv.open does not seem to exist, and I didn't bother to test the behavior.

TypeScript users might still get trapped even if the property is non-enumerable.

Oh, interesting, investigating...

I was able to reproduce the autocomplete behaviour, but for me it is coming from GitHub Copilot. With GitHub Copilot active I see argv["no-open"] as a suggestion in JavaScript or TypeScript having used it [sic] with opts.boolean. With it disabled, I do not get that suggestion.

I checked @types/minimist and there is not any attempt to infer the option names for typing the parse result.

So still an interesting detail, but different source of autocomplete.

@shadowspawn
Copy link
Collaborator

I will say that it seems reasonable to me to have no-open set to false (but be non-enumerable) AND open set to true, since that's reasonable, consistent, and would guard against that particular bug. @shadowspawn thoughts?

An intriguing idea, but too magical for my liking. It would certainly confuse me if I noticed that an option was set in an "invisible" way compared with logging.

(See also next comment.)

@shadowspawn
Copy link
Collaborator

shadowspawn commented Oct 12, 2023

In this case, parsing --no-foo to store false for option foo is the limit of the feature. Using no-foo with opts.boolean or any of the other configuration does not get special behaviour.

Commander and Yargs both have support for parsing --no-foo, and both have the same potential confusion that you can access no-foo on the parse results and be disappointed.

My preferred approach for now is still to add this to the documentation and not change the code. However, this has been a useful discussion and I have moved #48 to draft so I can take another look and extend what I wrote. The extra comments and discussion here make me think it deserves some text and not just buried in the big example.

shadowspawn added a commit to shadowspawn/minimist that referenced this issue Oct 12, 2023
Also, remove old security warning.

Fixes: minimistjs#38
shadowspawn added a commit to shadowspawn/minimist that referenced this issue Oct 13, 2023
Also, remove old security warning.

Fixes: minimistjs#38
shadowspawn added a commit to shadowspawn/minimist that referenced this issue Nov 25, 2023
Also, remove old security warning.

Fixes: minimistjs#38
shadowspawn added a commit to shadowspawn/minimist that referenced this issue Mar 1, 2024
Also, remove old security warning.

Fixes: minimistjs#38
@ljharb ljharb closed this as completed in dc69694 Mar 2, 2024
@shadowspawn
Copy link
Collaborator

For interest, I saw that Deno 1.23 switched to --no-* support being opt-in with negatable. The motivation appears to have been for improved typing, rather than a direct request for change in default behaviour. Their implementation is based on Minimist.

@shadowspawn
Copy link
Collaborator

shadowspawn commented May 11, 2024

For interest, the undocumented --no-* was reported as an issue on the original Minimist repo. Comments from about 4 users.

document "--no-XXX" handling
https://web.archive.org/web/20200904202930/https://github.com/substack/minimist/issues/54


I had to read the source code to diagnose why --no-foobar was not working as I expected (it wasn't giving me a "--no-foobar": true entry in my arguments. It was instead setting "foobar": false. Interesting feature, but should be documented.

@shadowspawn
Copy link
Collaborator

Another related issue on original Minimist repo:

Truncate --no-* for args
https://web.archive.org/web/20200904203615/https://github.com/substack/minimist/issues/123/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation help wanted pull requests welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants