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

Merge with config when array #83

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

markbirbeck
Copy link
Contributor

This is a fix for yargs/yargs#817.

@markbirbeck
Copy link
Contributor Author

Shouldn't the coverage test just check the entire branch, rather than individual steps? I'm assuming that the coverage number appears to fall because I add my test in one commit before I add the code that passes that test in the next. This makes it easier to review.

@bcoe
Copy link
Member

bcoe commented Mar 22, 2017

@markbirbeck thanks for the contribution (regarding coverage, this is a change in how coveralls works will not block the merging of this pull request).

@bcoe bcoe requested review from maxrimue and nexdrew March 22, 2017 06:26
@markbirbeck
Copy link
Contributor Author

Thanks for the update @bcoe.

Copy link
Member

@maxrimue maxrimue left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@bcoe
Copy link
Member

bcoe commented Apr 15, 2017

@markbirbeck sorry to drop this on the floor for so long -- just working through a large backlog of issues now; having slept on this one. It comes to mind that we might want to make this a parser option that can be enabled or disabled. Here's a scenario I frequently run into:

  • I'm configuring a cluster of servers, which are represented by an array; ['staging-www-1, 'staging-www-2] -- I might have this stored in a config.json.
  • In production, I might wish to override this value with ['prod-www-1', 'prod-www-2'] -- with this new logic we'd instead end up with ['staging-www-1', 'staging-www-2', 'prod-www-1', 'prod-www-2']?

Perhaps we simply add a new config option, combine-arrays, which defaults to false; if turned on it would combine merge arrays:

  • if multiple arrays are given as arguments: --arr 1 2 --arr 3 4.
  • or if a config file is provided along with an argument.

@bcoe
Copy link
Member

bcoe commented Apr 30, 2017

@markbirbeck does my last comment make sense? still interested in landing this?

@bcoe bcoe merged commit 806ddd6 into yargs:master Dec 20, 2017
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.

3 participants