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
Merged

Conversation

furudean
Copy link
Contributor

@furudean furudean 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.

@furudean furudean changed the title Add support yaml configuration file Add support for yaml configuration file Apr 4, 2019
@coveralls
Copy link

coveralls 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
Copy link
Member

JaKXz 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?

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@furudean
Copy link
Contributor Author

furudean commented Apr 5, 2019

@coreyfarrell Added! 👍

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.

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
Copy link
Member

bcoe commented Apr 9, 2019

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

@bcoe
Copy link
Member

bcoe 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.

@furudean
Copy link
Contributor Author

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

@coreyfarrell
Copy link
Member

@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
Copy link
Collaborator

isaacs 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
Copy link
Member

bcoe 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.

@furudean
Copy link
Contributor Author

I have incorporated changed based on our discussions

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.

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

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

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
@furudean furudean 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants