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

Fix issue with numeric character in group of options #19

Merged
merged 1 commit into from Aug 7, 2016

Conversation

Projects
None yet
5 participants
@elas7
Member

elas7 commented Apr 3, 2016

When checking a group of options, if there is any number in the characters following the key being currently evaluated, then everything after the current key will be set as its value. This happens because the regex that checks for numbers is missing a ^ at the beginning, so strings like 'abc123' are considered numbers.

An issue caused by this is that, for example, calling -abc=123 results in {"_": [], "a": "bc=123"} instead of {"_": [], "a": true, "b": true, "c": 123}

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 3, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d527a45 on elas7:fix-group into 48b1e6a on yargs:master.

coveralls commented Apr 3, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d527a45 on elas7:fix-group into 48b1e6a on yargs:master.

@@ -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' ])

This comment has been minimized.

@bcoe

bcoe Apr 9, 2016

Member

part of me would expect this to equal:

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

@bcoe

bcoe Apr 9, 2016

Member

part of me would expect this to equal:

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

This comment has been minimized.

@nexdrew

nexdrew Apr 9, 2016

Member

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
@nexdrew

nexdrew Apr 9, 2016

Member

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

This comment has been minimized.

@bcoe

bcoe Apr 9, 2016

Member

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}

@bcoe

bcoe Apr 9, 2016

Member

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}

This comment has been minimized.

@maxrimue

maxrimue Apr 9, 2016

Member

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?

@maxrimue

maxrimue Apr 9, 2016

Member

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?

This comment has been minimized.

@elas7

elas7 Apr 12, 2016

Member

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?

@elas7

elas7 Apr 12, 2016

Member

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?

var argv = parser([ '-ab5n=123' ])
argv.should.have.property('a', true)
argv.should.have.property('b', true)
argv.should.have.property('5', true)

This comment has been minimized.

@bcoe

bcoe Apr 9, 2016

Member

I think, given the test above, I would expect:

ab5n = 123

@bcoe

bcoe Apr 9, 2016

Member

I think, given the test above, I would expect:

ab5n = 123

This comment has been minimized.

@nexdrew

nexdrew Apr 9, 2016

Member

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).

@nexdrew

nexdrew Apr 9, 2016

Member

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).

This comment has been minimized.

@elas7

elas7 Apr 12, 2016

Member

@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.

@elas7

elas7 Apr 12, 2016

Member

@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.

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe Apr 9, 2016

Member

@nexdrew mind lending @elas7 and I a second set of eyes?

Member

bcoe commented Apr 9, 2016

@nexdrew mind lending @elas7 and I a second set of eyes?

@bcoe bcoe added the Next Major label May 1, 2016

@bcoe

This comment has been minimized.

Show comment
Hide comment
@bcoe

bcoe May 1, 2016

Member

@elas7 you've swayed me in your argument for how parsing should be applied. I think it's worth kicking this change to the next major release of yargs however, it has the potential to change parsing behavior in a way that is hard for folks to debug.

Member

bcoe commented May 1, 2016

@elas7 you've swayed me in your argument for how parsing should be applied. I think it's worth kicking this change to the next major release of yargs however, it has the potential to change parsing behavior in a way that is hard for folks to debug.

@maxrimue maxrimue referenced this pull request Jul 9, 2016

Closed

Version 5.0.0 #519

10 of 10 tasks complete

@bcoe bcoe merged commit f743236 into yargs:master Aug 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment