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: reporting watermarks #469

Merged
merged 1 commit into from
Jan 16, 2017
Merged

feat: reporting watermarks #469

merged 1 commit into from
Jan 16, 2017

Conversation

xdissent
Copy link
Member

Passes watermarks from the nyc config to istanbul-lib-report. This should allow custom watermarks for all reporters in istanbul-reports that use them. In the old .istanbul.yml the watermarks lived under a reporting key, so maybe this should be there too, but I read thru some of the other PRs about the new config and thought this might make more sense in the context of those discussions. Plus reportDir was already top level too.

@coveralls
Copy link

coveralls commented Dec 10, 2016

Coverage Status

Changes Unknown when pulling 25b98bd on xdissent:watermarks into ** on istanbuljs:master**.

@bcoe
Copy link
Member

bcoe commented Dec 11, 2016

@xdissent looking good to me. two asks for you:

  1. would it be possible to add a test? an integration test using the bin test suite is great.
  2. I think we should document this config setting in https://github.com/istanbuljs/nyc/blob/master/lib/config-util.js#L66

@xdissent
Copy link
Member Author

@bcoe I looked at the tests yesterday and I didn't really see a way to test the watermarks config's effect on anything. It's kinda difficult to add in a custom reporter at the moment, and none of the stock reporters seem to output anything differently based on watermarks - except colors, and colors don't appear to be used in the test output. Any suggestions? For the documentation, I also hit a snag because the watermarks config is an object and yargs isn't very helpful with object args IIRC. Istanbul didn't have cli options for the watermarks either, so I might need some guidance on how that should look if they need to be cli opts now too.

@bcoe
Copy link
Member

bcoe commented Dec 27, 2016

@xdissent haven't forgotten about this pull, but am digging myself out of a backlog of open-source work; hope to take a look at how one would test this soon 👍

@bcoe bcoe merged commit 0a1d72a into istanbuljs:master Jan 16, 2017
@bcoe
Copy link
Member

bcoe commented Jan 16, 2017

@xdissent thanks for the contribution, hopefully the first of many. I'm going to try to have a candidate release out today.

@bcoe
Copy link
Member

bcoe commented Jan 17, 2017

@xdissent would love your help testing:

npm cache clear; npm i nyc@next

If everything looks good in our smoke tests I will promote the release soon. Thanks for the contribution, I look forward to more in the future 👍

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

3 participants