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.

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

Jump to bottom

Fix issue with numeric character in group of options #19

Merged
merged 1 commit into from
Aug 7, 2016
Merged

Fix issue with numeric character in group of options #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,9 @@ function parse (args, opts) {
continue
}

// current letter is an alphabetic character and next value is a number
if (/[A-Za-z]/.test(letters[j]) &&
/-?\d+(\.\d*)?(e-?\d+)?$/.test(next)) {
/^-?\d+(\.\d*)?(e-?\d+)?$/.test(next)) {
setArg(letters[j], next)
broken = true
break
Expand Down
23 changes: 23 additions & 0 deletions test/yargs-parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,29 @@ describe('yargs-parser', function () {
argv.should.have.property('n', 123)
})

it('should set n to the numeric value 123, with n at the end of a group', function () {
var argv = parser([ '-ab5n123' ])
Copy link
Member

Choose a reason for hiding this comment

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

part of me would expect this to equal:

a: true
b: true
5: true
n: true
1: true
2: true
3: true

Copy link
Member

Choose a reason for hiding this comment

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

This is an odd edge case. I would expect something like -x1 to be parsed as x: 1, which I think is common among most POSIX bins, but I don't know what the typical interpretation of -x1y5 is.

In this test case, I would expect the result to be one of the following:

  1. a: true, b: 5, n: 123
  2. a: true, b: true, 5: true, n: 123

Copy link
Member

Choose a reason for hiding this comment

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

To me, option .2. feels a little more useful, I can't think of a situation where I want 5: true but I can imagine a lazy person writing -x1y5 and expecting:

{x: 1, y:5}

Copy link
Member

Choose a reason for hiding this comment

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

I would honestly expect it to be parsed like @bcoe noted in the first comment. I've actually not seen it in other bins, but I would expect grouping short options to not offer assigning values at all, because it's not really obvious to which options the values shall apply. As an example:
-xy1 could mean:

  1. Assign 1 to x and y, or
  2. Assign 1 to y and true to x

Even if we would document it, it's just not obvious if you read something like this.
Additionally, currently we do assigning values to options with either a = or a space:
--value=1 and --value 1
if we now allowed typing values of a certain kind right after the name of the option, wouldn't that kinda break the consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @maxrimue that we should better not assign values on short option groups based on kinds of values, most of the time it is not really obvious what the user is trying to do.

Yet, the current behavior is that numbers at the end of an options group are set as the value of the last key. Maybe we could add a configuration to yargs-parser to toggle that off?

argv.should.have.property('a', true)
argv.should.have.property('b', true)
argv.should.have.property('5', true)
argv.should.have.property('n', 123)
argv.should.have.property('_').with.length(0)
})

it('should set n to the numeric value 123, with = as separator', function () {
var argv = parser([ '-n=123' ])
argv.should.have.property('n', 123)
})

it('should set n to the numeric value 123, with n at the end of a group and = as separator', function () {
var argv = parser([ '-ab5n=123' ])
argv.should.have.property('a', true)
argv.should.have.property('b', true)
argv.should.have.property('5', 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, given the test above, I would expect:

ab5n = 123

Copy link
Member

Choose a reason for hiding this comment

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

I kind of agree. This seems to be an alternate form of --ab5n=123 since you can typically do -abc=123 as an alternate to --abc=123 (I think).

Copy link
Member Author

Choose a reason for hiding this comment

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

@bcoe @nexdrew: Currently, you should get ab5n = 123 only if the 'short-option-groups' configuration is turned off. That way, the -abc=123 and --abc=123 are considered the same.

With 'short-option-groups' turned on, I think it's expected behavior that every letter in a short option group is considered a separate option.

argv.should.have.property('n', 123)
argv.should.have.property('_').with.length(0)
})

it('should set option "1" to true, option "2" to true, and option "3" to numeric value 456', function () {
var argv = parser([ '-123', '456' ])
argv.should.have.property('1', true)
Expand Down