Skip to content

Add new type 'number' #5

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Add new type 'number' #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bdb282f
Add new type 'number'
maxrimue Feb 2, 2016
b554949
Add support for type 'number' in isNumber()
maxrimue Feb 8, 2016
4da2b52
Use isNaN() instead of Number() === NaN
maxrimue Feb 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ Parses command line arguments returning a simple mapping of keys and values.
* `opts.narg`: specify that a key requires `n` arguments: `{narg: {x: 2}}`.
* `opts.normalize`: `path.normalize()` will be applied to values set to this key.
* `opts.string`: keys should be treated as strings (even if they resemble a number `-x 33`).
* `opts.number`: keys should be treated as numbers.

**returns:**

Expand Down
8 changes: 8 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ function parse (args, opts) {
arrays: {},
bools: {},
strings: {},
numbers: {},
counts: {},
normalize: {},
configs: {},
Expand All @@ -45,6 +46,10 @@ function parse (args, opts) {
flags.strings[key] = true
})

;[].concat(opts.number).filter(Boolean).forEach(function (key) {
flags.numbers[key] = true
})

;[].concat(opts.count).filter(Boolean).forEach(function (key) {
flags.counts[key] = true
})
Expand Down Expand Up @@ -522,6 +527,7 @@ function parse (args, opts) {
var def = {
boolean: true,
string: '',
number: undefined,
array: []
}

Expand All @@ -533,12 +539,14 @@ function parse (args, opts) {
var type = 'boolean'

if (flags.strings && flags.strings[key]) type = 'string'
else if (flags.numbers && flags.numbers[key]) type = 'number'
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
    }

if (!configuration['parse-numbers']) return false
if (typeof x === 'number') return true
if (/^0x[0-9a-f]+$/i.test(x)) return true
Expand Down
14 changes: 14 additions & 0 deletions test/yargs-parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,20 @@ describe('yargs-parser', function () {
x.should.be.a('string').and.equal('56')
})

it('should default numbers to undefined', function () {
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.

})

it('should default number to NaN if value is not a valid number', function () {
var n = parser([ '-n', 'string' ], {
number: ['n']
}).n
expect(n).to.equal(NaN)
})

// Fixes: https://github.com/bcoe/yargs/issues/68
it('should parse flag arguments with no right-hand-value as strings, if defined as strings', function () {
var s = parser([ '-s' ], {
Expand Down