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.

Sign up for GitHub

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

Jump to bottom

Add support for relative path option #1212

Merged
merged 4 commits into from Dec 17, 2019
Merged

Add support for relative path option #1212

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e08a3e
Add support for relative path option
Oct 23, 2019
6240f4b
Merge remote-tracking branch 'upstream/master'
Nov 19, 2019
a7d31ef
use projectRoot instead of relative
Nov 20, 2019
ca85273
Update index.js
bgalletbgallet Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions index.js
View file
Open in desktop
Expand Up @@ -428,6 +428,7 @@ class NYC {
reports.create(_reporter, {
skipEmpty: this.config.skipEmpty,
skipFull: this.config.skipFull,
relative: this.config.relative === 'true',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
relative: this.config.relative === 'true',
projectRoot: this.cwd,

Copy link
Contributor Author

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 ?

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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.

maxCols: process.stdout.columns || 100
}).execute(context)
})
Expand Down