minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

Truncated, falsy arguments starting with --no #38

Closed andrewmclagan closed 5 months ago

andrewmclagan commented 1 year ago
$ 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 }
shadowspawn commented 1 year ago

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 commented 1 year ago

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

andrewmclagan commented 1 year ago

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

ljharb commented 1 year ago

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

shadowspawn commented 1 year ago

For interest, same issue in a Minimist fork: https://github.com/meszaros-lajos-gyorgy/minimist-lite/issues/10

JonnyBurger commented 10 months ago

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

ljharb commented 10 months ago

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

JonnyBurger commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 4 months ago

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 commented 2 months ago

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 commented 2 months ago

Another related issue on original Minimist repo:

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