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.

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

feat: allow disabling duplicate-arguments-array #67

Closed
wants to merge 1 commit into from

Conversation

laggingreflex
Copy link
Contributor

For yargs#530

Lets you disable "duplicates" behaviour

yargsParser('-x 1 -x 2')
// => {x: [1, 2]}
yargsParser('-x 1 -x 2', {configuration:{'duplicate-arguments-array': false}})
// => {x: 2}

@@ -544,8 +545,10 @@ function parse (args, opts) {
o[key] = value
} else if (Array.isArray(o[key])) {
o[key].push(value)
} else {
} else if (configuration['duplicate-arguments-array']) {
Copy link
Member

Choose a reason for hiding this comment

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

Great, that's about as simple as I was hoping it would be 👍

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

@laggingreflex awesome! thanks for the contribution.

If you wouldn't mind adding a test, so that we stay at 100% coverage, and adding a note about this configuration option in the docs, we'll get this landed ASAP.

@laggingreflex
Copy link
Contributor Author

This has a slight problem though... it requires you to put {"yargs": {"duplicate-arguments-array": false}} in package.json due to how yargs passes "configuration" to yargs-parser. Any suggestions to specify this config elsewhere? Or should it be handled in yargs?

@bcoe
Copy link
Member

bcoe commented Nov 14, 2016

@laggingreflex you can also manually pass configuration to yargs-parser, which is how yargs actually does it 😄

https://github.com/yargs/yargs-parser/blob/master/test/yargs-parser.js#L1777

☝️ you should be able to crib that test.

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Nov 14, 2016

Can the consumers of yargs be able to pass that configuration to yargs-parser though? Because I originally intended to fix yargs#530 with this.

import yargs from 'yargs'

const config = yargs(???).parse('-x 1 -x 2');

It seemed to me the only way for a yargs user to pass this configuration would be by using package.json way - due to how yargs passes "configuration" to yargs-parser ... unless I'm missing something?

@bcoe
Copy link
Member

bcoe commented Nov 14, 2016

@laggingreflex yargs' is smart enough to read configuration from your projects own package.json; so once you add this setting, a user who wants this behavior, simply needs to add:

{ "yargs": { "duplicate-arguments-array": false } }

To their project's own package.json.

Lets you disable "duplicates" behaviour

```
yargsParser('-x 1 -x 2')
// => {x: [1, 2]}
```
```
yargsParser('-x 1 -x 2', {configuration:{'duplicate-arguments-array': false}})
// => {x: 2}
```
@bcoe
Copy link
Member

bcoe commented Nov 26, 2016

@laggingreflex I'm really happy with this, will work on getting this landed ASAP.

@bcoe
Copy link
Member

bcoe commented Nov 27, 2016

@laggingreflex mind rebasing, I think I landed your other pull request in the wrong order.

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Nov 27, 2016

That one (#71) included this one (#67) so now both are applied, however the resultant commit's message (0f0fb2d) only reflects #71 (changes from both are still applied though). Would rebasing still help?

@bcoe
Copy link
Member

bcoe commented Nov 28, 2016

@laggingreflex if you checkout master, and rebase this branch against it, it'll do the write thing and make it so this can be landed -- If you don't, I can rebase on another branch for you, but it changes the commit history a bit.

@laggingreflex
Copy link
Contributor Author

laggingreflex commented Nov 28, 2016

This already has been landed.

#71 included commits for both this (#67) and itself so as far as the matter is of landing this PR, it has already been done; it was included in #71 and the changes have landed successfully.

The only (minor?) problem is that the two commits have been squashed into one 0f0fb2d which makes no mention of this PR (#67), but only that of #71 (in the commit message). If it's not a big problem then you may close this PR. If however you do want separate commits then I think you probably will need to re-write history a bit. I did try to rebase but it gives me "nothing to commit", because like I said, the changes have already landed in latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants