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: Add support for yaml configuration file #1054

Merged
merged 10 commits into from Apr 16, 2019

Conversation

@c-bandy
Copy link
Contributor

commented Apr 4, 2019

I added support for .yml and .yaml config files as per request in #892.

I wasn't sure how to implement tests for this, so suggestions are welcome.

@c-bandy c-bandy changed the title Add support yaml configuration file Add support for yaml configuration file Apr 4, 2019

@coveralls

This comment has been minimized.

Copy link

commented Apr 4, 2019

Coverage Status

Coverage increased (+0.02%) to 96.667% when pulling 362eff0 on c-bandy:master into d7a9d6a on istanbuljs:master.

@JaKXz
JaKXz approved these changes Apr 4, 2019
@JaKXz

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@c-bandy thank you for the PR! would you mind adding to the README where it mentions the different kinds of rc files you can have?

@coreyfarrell
Copy link
Member

left a comment

I would like to see some basic tests for this. To do so please:

  1. Add js-yaml as a dev dependency.
  2. See test/nyc-integration.js at about line 445 (search for describe('.nycrc', function () {). This is where we have testing for .nycrc. I think we should have similar tests for js-yaml.
lib/config-util.js Outdated Show resolved Hide resolved
@coreyfarrell
Copy link
Member

left a comment

Just a couple minor coverage related issues then this should be good to merge.

package.json Outdated Show resolved Hide resolved
lib/config-util.js Outdated Show resolved Hide resolved
lib/config-util.js Outdated Show resolved Hide resolved
@coreyfarrell

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

@c-bandy Also please mention these new configuration files in our README.md. See https://github.com/istanbuljs/nyc#configuring-nyc

c-bandy added 4 commits Apr 5, 2019
Doc
@c-bandy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@coreyfarrell Added! 👍

@bcoe
Copy link
Member

left a comment

if we are going to support multiple yaml parsers, let's pull the logic for loading the yaml parser into its own module lib/yaml ...

I'd rather just support one yaml parser. We're opening ourselves up to config bugs that exist in one parser but not the other, I'd rather be less democratic and pick one.

lib/config-util.js Outdated Show resolved Hide resolved
@bcoe

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@c-bandy really appreciate this contribution 😄 the tests certainly seem like they're on the right track.

@bcoe

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

@c-bandy I missed that both yaml parsers are optional (hadn't noticed js-yaml was a developer dependency); I'm more open to checking for both 👍still might be nice to encapsulate the logic that checks for yaml parsers into a lib/yaml.js parser ... would be neat to potentially consider adding a similar toml.js parser as a follow on.

@c-bandy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

@bcoe Thanks for your feedback! I will look into making these changes today

@coreyfarrell

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@c-bandy please hold off for a bit, we've been having a discussion on our slack about the best way to handle yaml and other formats. I don't think we've come to a consensus yet. At this point we are focused on getting nyc@14.0.0 released. YAML config will be a non-breaking change so we can consider it for 14.1.0.

@isaacs

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Copying here my suggestion from the discussion elsewhere: since so many testing libraries include js-yaml anyway, it's probably best to just declare that one as a dep and use it.

@bcoe

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@c-bandy sorry to be a pain, I know it's annoying when a PR is blocked by folks not being able to reach consensus. If you'd like to join in with the conversation, we actually have a slack here:

http://devtoolscommunity.herokuapp.com/

Would love to get your opinion as we opine.

c-bandy added 2 commits Apr 12, 2019
@c-bandy

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I have incorporated changed based on our discussions

@bcoe
bcoe approved these changes Apr 12, 2019
Copy link
Member

left a comment

this is looking good to me, I like the introduction of the lookup table.

@coreyfarrell
Copy link
Member

left a comment

I'm approving this but not sure it should be merged right now or wait until 14.0.0 is released.

@JaKXz JaKXz merged commit ca37ffa into istanbuljs:master Apr 16, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 96.667%
Details

@c-bandy c-bandy changed the title Add support for yaml configuration file feat: Add support for yaml configuration file Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.