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

Add new type 'number' #5

Closed
wants to merge 3 commits into from
Closed

Add new type 'number' #5

wants to merge 3 commits into from

Conversation

maxrimue
Copy link
Member

@maxrimue maxrimue commented Feb 2, 2016

This adds a new type number.

A number-type option has the following special behaviour:

  • If the flag itself is not provided, its value will default to undefined, unless defined otherwise via .default()
  • If no number is provided as a value, its value will default to NaN (that means, if Number() isn't capable of coercing the value to a number primitive)

Note: Where should I implement the logic for checking if the given value is a number? I thought of the following code:

if (typeof x === 'number') {
  return x
} else {
  return Number(x) // Will return NaN if x is not coercible to a number
}

I am aware of isNumber(), however, it only returns true if the option parse-numbers equals false, which is obviously not related to the number-type. What do folks think?

var n = parser([ '-n' ], {
number: ['n']
}).n
expect(n).to.equal(undefined)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nexdrew what are your thoughts about defaulting a number to undefined ... but one could also argue for NaN or 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaulting to undefined is better imo. Dealing with NaN has always been a pain in the butt in my experience.

@bcoe
Copy link
Member

bcoe commented Feb 6, 2016

@maxrimue this is looking good so far, need any help getting tests passing?

else if (flags.arrays && flags.arrays[key]) type = 'array'

return type
}

function isNumber (x) {
if (flags.numbers && flags.numbers[x] && !isNaN(x)) return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can get rid of this check, it won't actually do anything, instead update:

https://github.com/yargs/yargs-parser/blob/master/index.js#L315

To be something like:

var value = val
if (!checkAllAliases(key, flags.strings)) {
  if (isNumber(val) || checkAllAliases(key, flags.numbers)) value = Number(val)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcoe @maxrimue got this going so far:

    if (!checkAllAliases(key, flags.strings)) {
      if (isNumber(val)) value = Number(val)
      if (!isNumber(val) && checkAllAliases(key, flags.numbers) && !isUndefined(val)) value = NaN
    }

@lrlna lrlna mentioned this pull request Feb 14, 2016
@lrlna
Copy link
Member

lrlna commented Feb 14, 2016

@maxrimue sorry -- I can't push to your branch for some reason!! I am opening a new Pull Request #7 to add that line of code.

@lrlna lrlna closed this Feb 14, 2016
@maxrimue maxrimue deleted the New-type-number branch February 14, 2016 09:35
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 this pull request may close these issues.

3 participants