-
Notifications
You must be signed in to change notification settings - Fork 120
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 type "array" with "duplicate-arguments-array" #163
Conversation
@juergba thank you for all the wonderful contributions you've made recently, I will work on getting them merged as soon as possible; I've been buried on other projects and apologize. |
@juergba could I bother you to rebase? I'm getting ready to release a new version of yargs and yargs-parser. |
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.
my thinking is, if you've explicitly set type to array
, we should still accept the option being set multiple times -- duplicate-arguments-array
applying only to the behavior of keys you haven't explicitly set as an array.
Honestly, I'd kind of like to drop support for -x a b c
, and make it so the only way to create an array is -x a -x b -x c
; since type array
does not mix well with positional arguments.
tldr; I think perhaps we need better documentation around arrays, and to perhaps rethink the behavior of the array
option, but I'm so so on this patch.
@coreyfarrell @plroebuck @boneskull have any opinions about array behavior? mainly my idea of deprecating -x 2 3 3
in favor of the more explicit -x 2 -x 3 -x 3
.
Agree, type
What about |
Yes please. In the context of nyc I would like to block use of
Maybe it would be better to disable |
I think
I agree with this, maybe a parameter called
@juergba does this sound reasonable to you for mocha's needs? If so perhaps we could update this patch to reflect this world. |
afaik we don't use
Ok, I will give it a try. I'm not sure yet about the implications of "disabling |
adapt existing tests, add new tests
@bcoe The behavior of option type
|
@juergba my thinking is we should make this a breaking change, and address some of these long standing issues. If you indicate something is an array, even a It might break the internet too much to drop support for |
I understand your concerns with The current master shows following results: var args = parse('--file --file', {
'array': ['file'],
'number': ['file'], // { _: [], file: [ 0, 0, undefined ] }
// 'string': ['file'], // { _: [], file: [ [], [], '' ] }
// 'boolean': ['file'], // { _: [], file: [ [], [], [] ] }
configuration: {
'duplicate-arguments-array': true,
'flatten-duplicate-arrays': false
}
})
var args = parse('--file true --file true', { // same result with '--file --file'
'boolean': ['file'],
configuration: {
'duplicate-arguments-array': true
}
}) // { _: [], file: true }
We should address those bugs first, before adding new flags. |
No, please don't close. This issue covers its own topic, which got lost completely during
Yes, landing fixes step by step is a good procedure. |
@juergba any chance we can dust this PR off, I'd like to start working on getting a few more releases of yargs out the door. |
Yes, I can make the original issue (type "array" with "duplicate-arguments-array") ready within a few days. The disabling of creating an array by |
@juergba sounds good to me, I think that's @coreyfarrell's recommendation to make disable |
@bcoe please confirm wether you really want above pattern: In #75 the opposite was implemented: |
@juergba if you have
If you have
This agrees with your thinking? |
No, this is not what I'm thinking. Neither after your comment:
Nor after @coreyfarrell 's comment:
@coreyfarrell can you help, please? |
@juergba Therefore, in the edge case that you've set a
I don't think we should change this behavior ... and it's pretty edge-casey to be honest (not something I've seen many issues about). What I would be more willing to entertain would be a redefinition of what setting the type tldr; I'm not super excited about the changes proposed in this PR because it seems like thrash without solving much of a problem I've observed, I'd rather work to make the |
getting back to your original PR description: If duplicate-arguments-array = false
If duplicate arguments-array = true
☝️ if there are any fixes we can make around these expectations let's make them 👌 And then I'd say, revisit the meaning of |
Meanwhile a few PR's refering |
@juergba 👍 sorry for this dragging for so long, FYI any chance I could get you to join the slack here: http://devtoolscommunity.herokuapp.com/ I'd like to roll out a new yargs with a bunch of your fixes, but would first like to make sure that it seems to behave nicely with |
With this change we add a Yargs middleware that normalizes non Array options when the argument has been provided multiple times. By default, when an option is non array and it is provided multiple times in the command line, yargs will not override it's value but instead it will be changed to an array unless `duplicate-arguments-array` is disabled. But this option also have an effect on real array options which isn't desired. See: yargs/yargs-parser#163 (comment) Closes #22956
Description
closes #162
The
array
type does not work consistently with optionduplicate-arguments-array
.when
duplicate-arguments-array === false
array
of typestring
does reduce duplicate argumentsarray
of typenumber
does not reduce duplicate argumentsarray
of typeboolean
only works with arguments provided in pairs, eg--foo true
.A
boolean
without value (--foo --no-foo
) results in various bugs as empty arrays, lost flags, lost array type and more.Description of Change
duplicate-arguments-array
has no effect on option typearray
anymore.even when
duplicate-arguments-array === false
, you are able to specify:(output depends on
flatten-duplicate-arrays
)-x 1 -x 2 -x 3
==>[1, 2, 3]
or[[1], [2], [3]]
-x 1 2 -x 4 5
==>[1, 2, 4, 5]
or[[1, 2], [4, 5]]
number
andstring
work consistently.array
value is always anarray
, even if just one value is passed.maybeCoerceNumber()
: fix handling ofarray
s.Please note: the disabling of creating an
array
by-x a b c
by default will be implemented in another PR.