-
Notifications
You must be signed in to change notification settings - Fork 123
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
Conversation
@@ -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' ]) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
a: true
,b: 5
,n: 123
a: true
,b: true
,5: true
,n: 123
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
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:
- Assign
1
tox
andy
, or - Assign
1
toy
andtrue
tox
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?
There was a problem hiding this comment.
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?
@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. |
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}