minimistjs / minimist

parse argument options
MIT License
530 stars 30 forks source link

Fix detection of numeric value when parsing short options #3

Open shadowspawn opened 1 year ago

shadowspawn commented 1 year ago

Problem

The parsing behaviour changes depending on whether the last character in group is a number, switching between a group of boolean options and an option taking a value. See https://github.com/minimistjs/minimist/issues/2#issue-1410232307

For example:

$ node example/parse.js -a1
{ _: [], a: 1 }

$ node example/parse.js -a1b
{ '1': true, _: [], a: true, b: true }

$ node example/parse.js -a1b2
{ _: [], a: '1b2' }

$ node example/parse.js -a1b2c
{ '1': true, '2': true, _: [], a: true, b: true, c: true }

Solution

Per https://github.com/minimistjs/minimist/issues/2#issuecomment-1279821340

There is a bug in the code detecting whether the remaining characters in the argument are a number. If the argument ends in a number it is treated as a number, even if there are letters before that.

The search /-?\d+(\.\d*)?(e-?\d+)?$/ is not anchored at the start and thinks 1b2 is a number because it ends in a number. So it treats -a1b2 like -a123 and assigns the "number" to the a option.

shadowspawn commented 1 year ago

This change makes the short option parsing work as intended, but changes the parse results and will be a breaking change for some users. (Albeit, as is any change!)

ljharb commented 1 year ago

How likely is it that anyone will have been relying on the incorrect, and untested, results?

shadowspawn commented 1 year ago

The potentially broken use case I have in mind is a short option value that for external reasons consistently ends in a number. A user might have discovered that the value could be put straight after the short option. For example:

enrol --course=COSC201
enrol -c COSC201
enrol -c=COSC201
enrol -cCOSC201   <-- only works like above due to bug

This might be rare since:

1) The README includes an example of short flags expanding into multiple individual flags, and using space separated values, so examples of how to use as intended. i.e. -x 3, -y 4, -n5, and -abc:

$ node example/parse.js -x 3 -y 4 -n5 -abc --beep=boop foo bar baz

2) The incorrect behaviour only happened for values ending in a number, so user might realise it was not a reliable way of supplying value even if they stumbled across it.

3) I don't recall issues mentioning the variant behaviour (but weak evidence, not sure how far back through the old open bugs I read or retained!).

shadowspawn commented 1 year ago

I checked what yargs does, as the most evolved optimist descendent:

try-yargs $ node index.js -ABC123               
{ _: [], A: true, B: true, C: 123, '$0': 'index.js' }

Old minimist:

try-minimist $ node index.js -ABC123
{ _: [], flag: false, A: 'BC123' }

And on this branch minimist does:

$ node index.js -ABC123               
{ _: [], flag: false, A: true, B: true, C: 123 }
ljharb commented 1 year ago

I agree this is better/more intuitive behavior. It would, however, be quite a breaking change.

shadowspawn commented 1 year ago

Marking as semver-major (copied the label text and colours from qs)

If this comes up a lot in issues, or if we decide never to do a major, can reconsider.

shadowspawn commented 4 months ago

This was reported as an issue on the original Minimist repo (comments from 2 users):

Parsing multiple short option with a default value https://web.archive.org/web/20200904203146/https://github.com/substack/minimist/issues/50


node ../minimist/example/parse.js -ab1

{ _: [], a: 'b1' }

But the behavior I was expecting was: { _: [], a: true, b: 1 }

shadowspawn commented 4 months ago

And a duplicate report on original Minimist repo:

Incorrectly parsed values while short param end with number https://web.archive.org/web/20200904203609/https://github.com/substack/minimist/issues/103/


minimist(['-a2']) // => { _: [], a: 2 }  

minimist(['-abc']) // => { _: [], a: true, b: true, c: true }

minimist(['-abc2']) // => { _: [], a: 'bc2' }  why not { _: [], a: true, b: true, c: 2 }?