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

Add support for relative path option #1212

Merged
merged 4 commits into from Dec 17, 2019

Conversation

bgalletbgallet
Copy link
Contributor

@bgalletbgallet bgalletbgallet commented Oct 23, 2019

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

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',
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.

@bgalletbgallet
Copy link
Contributor Author

@coreyfarrell Everything should be ok, the issue with --cwd option is still present but this PR is not concerned anymore.
By default the lcov report is relative. A customer projectRoot option can be used to change the 'from' argument of the path.relative() function

@coreyfarrell
Copy link
Member

@bcoe This adds a projectRoot option to nyc which could be set differently than cwd. Do you think this is useful? I'm concerned this could cause confusion for users.

Ref #1234 - the config loading is complicated and potentially needs some tweaks before we do a final release of nyc 15.

@bcoe
Copy link
Member

bcoe commented Nov 22, 2019

@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

@coreyfarrell
Copy link
Member

@bgalletbgallet could you explain to me why we need a separate option for projectRoot compared to cwd? I'm very concerned this will cause confusion to end users, and interested in knowing about a scenario where the value of cwd will would be unacceptable as the base of reporting. Keep in mind that by default cwd will be the directory containing the package.json which applies to process.cwd(). I'm afraid to add complexity without understanding the justification.

A minimal reproduction which installs git://github.com/bgalletbgallet/nyc#master for the nyc dependency showing that it is necessary to set projectRoot to get functional results would be great.

@bgalletbgallet
Copy link
Contributor Author

@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

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

index.js Outdated Show resolved Hide resolved
index.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.

Thanks for the contribution and for making the updates!

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