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

nyc command line include parameter fix #84

Closed
wants to merge 4 commits into from
Closed

nyc command line include parameter fix #84

wants to merge 4 commits into from

Conversation

alpersogukpinar
Copy link
Collaborator

nyc command line include parameter was being ignored and package.json nyc/include values were being used everytime.
Fix #82

nyc command line include parameter was ignored and package.json nyc/include values were used everytime. Fixed that problem.
Fix #82
nyc command line include parameter was being ignored and package.json nyc/include values were being used everytime. Fixed that problem.
Fix #82
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.0% when pulling 0020b37 on alpersogukpinar:master into 86c1c09 on istanbuljs:master.

@bcoe
Copy link
Member

bcoe commented Jan 16, 2017

@alpersogukpinar nice approach 👍 NYC_CONFIG is a relatively new environment variable; I was passing around 1,000,001 NYC_ prefixed variables, which were simplified into this one config variable.

exclude = testExclude(assign(
{ cwd },
Object.keys(opts).length > 0 ? opts : {
include: include,
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd prefer that we only set include if NYC_CONFIG.include has been found; perhaps pull this object out into an object called defaults; and only set the include key if it's found?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.0% when pulling e55b6dc on alpersogukpinar:master into 86c1c09 on istanbuljs:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.0% when pulling e55b6dc on alpersogukpinar:master into 86c1c09 on istanbuljs:master.

@alpersogukpinar
Copy link
Collaborator Author

@bcoe I had tried to do something similar by setting include = false if process.env.NYC_CONFIG is undefined. I hope, this time I've changed the code as you suggested, if not please be more specific about the change :-)

@bcoe
Copy link
Member

bcoe commented Jan 23, 2017

@alpersogukpinar haven't forgotten about this, and am hoping to have some OSS cycles in the coming week (have been on a binge working on a new major version of yargs recently).

@bcoe
Copy link
Member

bcoe commented Feb 7, 2017

@alpersogukpinar merged this into a slightly different branch with tests, will have pull in a second 👍

@bcoe
Copy link
Member

bcoe commented Feb 7, 2017

@alpersogukpinar could you please give this a try:

npm cache clear; npm i babel-plugin-istanbul@next

I've landed your work, with a couple minor tweaks (including adding a couple tests for us).

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