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

Arguments parsed to numbers incorrectly. #24

Closed
cowboy opened this issue Sep 11, 2013 · 10 comments · Fixed by #34
Closed

Arguments parsed to numbers incorrectly. #24

cowboy opened this issue Sep 11, 2013 · 10 comments · Fixed by #34

Comments

@cowboy
Copy link

cowboy commented Sep 11, 2013

I know this is going to be a little bit subjective, but perhaps you can help.

It's hard to pass a version number to nopt like -r=1.10 because it will treat that value as the number 1.1 and not the string "1.10". This can be worked around by prefixing it like -r=v1.10 but it's inconvenient to have to strip out the leading v in that case if you don't need it.

Would it be possible for nopt to check to see if the value, once converted to a number and then back to a string matches the original string, Eg.

function getNumberOrString(str) {
  var num = Number(str);
  // Only return number if a) it's a valid number, and b) when coerced
  // back to a string, that string is the same as the original string.
  return !isNaN(num) && String(num) === str ? num : str;
}

getNumberOrString("1.23") // 1.23
getNumberOrString("1.10") // "1.10"
getNumberOrString("01234") // "01234"

What nopt currently does is something more like this:

function getNumberOrString(str) {
  var num = Number(str);
  // Only return number if it's a valid number.
  return !isNaN(num) ? num : str;
}

getNumberOrString("1.23") // 1.23
getNumberOrString("1.10") // 1.1
getNumberOrString("01234") // 1234

Here's an example of this "bug" in action:

$ npm install nopt@latest
npm WARN package.json foo@0.0.0 No README.md file found!
npm http GET https://registry.npmjs.org/nopt
npm http 304 https://registry.npmjs.org/nopt
npm http GET https://registry.npmjs.org/abbrev
npm http 304 https://registry.npmjs.org/abbrev
nopt@2.1.2 node_modules/nopt
└── abbrev@1.0.4

$ node -pe "require('nopt')({}, {}, ['-a=1.23', '-b=1.10', '-c=01234'], 0)"
{ a: 1.23,
  b: 1.1,
  c: 1234,
  argv:
   { remain: [],
     cooked: [ '-a', '1.23', '-b', '1.10', '-c', '01234' ],
     original: [ '-a=1.23', '-b=1.10', '-c=01234' ] } }
@cowboy
Copy link
Author

cowboy commented Sep 11, 2013

FWIW, jQuery did something like this at one point trying to make their data- attribute parsing smart and it ended up being a bit of a mess.

@michaelficarra
Copy link
Contributor

Just specify it to be a string by providing knownOpts. Your proposed behaviour would not be desirable for me since I often use scientific notation to specify numeric values (e.g. -n 1e6).

@cowboy
Copy link
Author

cowboy commented Sep 11, 2013

@michaelficarra In this case, these are unexpected arguments.

@cowboy
Copy link
Author

cowboy commented Sep 11, 2013

If unexpected were always stored as strings (maybe an option could be added to enable this) then it could be up to the end-user how they wanted to coerce or parse that string value. Which would solve both problems, I think.

@michaelficarra
Copy link
Contributor

Yes, I could agree with that.

@cowboy
Copy link
Author

cowboy commented Sep 11, 2013

This is how I'd do it. Create an option whereby a "reviver" method (like JSON.parse has) can be specified for unknown values, with the default being the current behavior. Then, if I want every unknown value to be a string, the reviver method I'd specify is function(s) { return s; }.

@isaacs
Copy link
Contributor

isaacs commented Sep 12, 2013

I could see adding a type that preserves trailing/leading zeroes like you describe. It shoudln't be too hard to do in your app with a custom type, but you'll probably have to look at the nopt and/or npm source to see how to go about it. Doc patch welcome.

@cowboy
Copy link
Author

cowboy commented Sep 12, 2013

I'm talking about unexpected arguments.

@isaacs
Copy link
Contributor

isaacs commented Sep 12, 2013

Oh, right. Sure, patch welcome.

@michaelficarra
Copy link
Contributor

(maybe an option could be added to enable this)

Please, no additional options. Just unconditionally treat unexpected values as strings.

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

Successfully merging a pull request may close this issue.

3 participants