minimistjs / minimist

parse argument options
MIT License
530 stars 30 forks source link

Ignore numeric-looking strings with huge values #18

Closed shadowspawn closed 1 year ago

shadowspawn commented 1 year ago

An issue people encounter using minimist (as reported on old repository) is that false-positive numeric-looking strings are unexpectedly converted to numbers with data loss, whether losing precision or being converted to infinity. For example, values for twitter or YouTube ids.

The README says:

Numeric-looking arguments will be returned as numbers unless opts.string or opts.boolean is set for that argument name.

The suggested author work-around of explicitly setting the option parsing does avoid the automatic number conversion for options, but this approach is not available for positional arguments.

A possible improvement is to follow the lead of yargs and not attempt to coerce numeric-looking strings which are too big to reasonably represent: https://github.com/yargs/yargs-parser/pull/110

(This only addresses one issue with automatic numeric conversion, and I don't mind if we leave it out and keep it simple! I had seen the issue reported and wanted to look into the problem, and how the code had evolved in Yargs which I knew had added some extra checks.)

codecov-commenter commented 1 year ago

Codecov Report

Merging #18 (abca18e) into main (5784b17) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files           1        1           
  Lines         161      162    +1     
  Branches       71       72    +1     
=======================================
+ Hits          159      160    +1     
  Misses          2        2           
Impacted Files Coverage Δ
index.js 98.76% <100.00%> (+<0.01%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shadowspawn commented 1 year ago

isSafeInteger was introduced with Node.js 0.12. If there is interest in this PR, can probably hand-code an equivalent which runs in older versions.

shadowspawn commented 1 year ago

I wanted to look into it, and have! Have some clues for future if needed. Not compelling enough for now.

shadowspawn commented 1 year ago

For interest, I found a copy of the original reported Minimist bug: https://github.com/meszaros-lajos-gyorgy/minimist-lite/issues/2

shadowspawn commented 8 months ago

The suggested author work-around of explicitly setting the option parsing does avoid the automatic number conversion for options, but this approach is not available for positional arguments.

Actually, can specify positional are "string" by specifying _: #52.

shadowspawn commented 3 months ago

I noticed this is touched on in the Open Group Base Specifications for Utility Conventions

  1. Unless otherwise specified, whenever an operand or option-argument is, or contains, a numeric value:
    • The number is interpreted as a decimal integer.
    • Numerals in the range 0 to 2147483647 are syntactically recognized as numeric values. ...

So there is some precedent for limiting the numbers, if we choose to in the future.