-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Add support for relative path option #1212
Conversation
index.js
Outdated
@@ -428,6 +428,7 @@ class NYC { | |||
reports.create(_reporter, { | |||
skipEmpty: this.config.skipEmpty, | |||
skipFull: this.config.skipFull, | |||
relative: this.config.relative === 'true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relative: this.config.relative === 'true', | |
projectRoot: this.cwd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @coreyfarrell
Shouldn't I use this.config.cwd to access the cwd option ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, please use this.cwd
. In some edge cases it's possible that this.config.cwd
might be unset but this.cwd has the default process.cwd()
.
https://github.com/istanbuljs/nyc/blob/master/index.js#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect I didn't see it was already set. I do it tonight
Thank you for your anwser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've altered the suggested change per my latest comment on the istanbuljs PR.
@coreyfarrell Everything should be ok, the issue with --cwd option is still present but this PR is not concerned anymore. |
@coreyfarrell this would be extremely useful to me, I'm currently floating a patch for the Node.js nightly builds, with almost this functionality (so that merging works): https://codecov.io/gh/nodejs/node/src/64ee64d3afc837350ee39058cef9179e92f3554c/lib/fs.js |
@bgalletbgallet could you explain to me why we need a separate option for A minimal reproduction which installs |
@coreyfarrell I don't know, I was about to delete it and @bcoe told that it was usefull to him. I don't see any case where you need a different projectRoot than the default cwd. If it's ok for @bcoe, we will delete it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe @bcoe was mixed up with the reviews, he's actually using the istanbul-lib-report
/ istanbul-reports
modules directly in c8.
As for nyc I would still like to have projectRoot: this.cwd
passed to the reporter options. If someone has a need for projectRoot
to be different from this.cwd
I can consider that as a separate non-breaking change, but I'd prefer to avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution and for making the updates!
Add the possibility to use --relative=true option in command line
This is useful if you run your test on a linux environment (docker, vm, server) and dev on a windows one.
If you want your coverage to work with your IDE, you need relative path instead of absolute ones.
Linked with pull request istanbuljs/istanbuljs#492