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

Fix detection of numeric value when parsing short options #3

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Oct 15, 2022

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 #2 (comment)

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 #2 (comment)

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
Copy link
Collaborator Author

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 ljharb force-pushed the feature/fix-numeric-short-options branch from 7564f07 to eac091b Compare October 16, 2022 21:03
@ljharb
Copy link
Member

ljharb commented Oct 16, 2022

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

@ljharb ljharb force-pushed the feature/fix-numeric-short-options branch from eac091b to e2dbb25 Compare October 16, 2022 21:09
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Oct 17, 2022

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

  2. 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
Copy link
Collaborator Author

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
Copy link
Member

ljharb commented Feb 5, 2023

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

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Feb 9, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants