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

Allow configuration to extend other config files #775

Closed
wants to merge 2 commits into from

Conversation

rcoy-v
Copy link
Member

@rcoy-v rcoy-v commented Feb 3, 2017

This is a solution to istanbuljs/nyc#503. I still need to test this with nyc, but I would like to get feedback early.

This only applies the extends functionality when .config() is given an object. Please let me know if there are other cases that should use this as well.

@bcoe
Copy link
Member

bcoe commented Feb 6, 2017

@rcoy-v thanks for this 👍 I'm currently digging out of a mound of OSS tickets, and am looking very much forward to getting to this one.

@bcoe bcoe requested review from JaKXz, maxrimue and elas7 February 6, 2017 20:59
@rcoy-v
Copy link
Member Author

rcoy-v commented Feb 6, 2017

I tested this through nyc with one of our internal projects. It fulfilled my requirement at least for extending configurations.

@bcoe
Copy link
Member

bcoe commented Feb 6, 2017

@rcoy-v my only concern is projects that happen to have an extends key in their configuration; @nexdrew is usually pretty good at giving feedback on these edge-cases.

we're marching towards a 7.x release, so it's a good time to add this feature.

@rcoy-v
Copy link
Member Author

rcoy-v commented Feb 6, 2017

I agree, this is essentially adding a new reserved word. I would treat it as a major semver change. I did not update the README in this pr to detail the extends keyword. I wanted to wait for feedback.

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.

LGTM 👍 Thanks for submitting!

[*.js]
end_of_line = lf
indent_style = space
indent_size = 2
Copy link
Member

Choose a reason for hiding this comment

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

this is great, thanks for adding it! Could you perhaps add insert_final_newline = true?

@bcoe
Copy link
Member

bcoe commented Feb 10, 2017

@rcoy-v mind rebasing this against the 7.x branch, and opening this pull against 7.x rather than master? marching towards this release.

@rcoy-v
Copy link
Member Author

rcoy-v commented Feb 10, 2017

Closed in favor of #779.

@rcoy-v rcoy-v closed this Feb 10, 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.

None yet

4 participants