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

→ Fix common cross-platform issue with file path normalization. #86

Merged
merged 4 commits into from
Aug 14, 2016

Conversation

afloyd
Copy link
Contributor

@afloyd afloyd commented Feb 12, 2016

Between gulp3 & gulp4 the way file paths are generated may have changed.

Reference: SBoudrias/gulp-istanbul/#48

…en gulp3 & gulp4 the way file paths may have changed.

Reference: SBoudrias/gulp-istanbul/SBoudrias#48
Re: SBoudrias/gulp-istanbul/SBoudrias#48
@@ -15,6 +15,11 @@ var PluginError = gutil.PluginError;
var PLUGIN_NAME = 'gulp-istanbul';
var COVERAGE_VARIABLE = '$$cov_' + new Date().getTime() + '$$';

function getNormalizedPath(filepath) {
Copy link
Owner

Choose a reason for hiding this comment

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

Pretty sure we could simply use https://www.npmjs.com/package/normalize-path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I probably should have named the function better... This is actually making sure the original platform file path separator is used. I think that normalize-path just makes everything use /, not sure if that would work or not. But I'll give it a try and see...

This isn't my project, so if it does work and you'd prefer to use an external dependency that's fine. But generally if what I'm needing from an external module is just a single line of code, it seems a bit of an excess to require external dependencies for that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plugged in the normalize-path module, but as I feared it didn't fully function. It did better than without it, but for some reason the number of statements passing coverage dropped by 50%.

Also, in the generated report the paths included the entire file path including hard drive letter rather than a relative path based on the root project directory. Versus using the original platform path separator which makes it work the same on Windows as it does on Mac/linux...

Copy link
Owner

Choose a reason for hiding this comment

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

let's give a better name to that function then. normalizePathSep() or something

@SBoudrias
Copy link
Owner

@afloyd hey, any chance to get the code fix so we can merge?

@SBoudrias
Copy link
Owner

SBoudrias commented Jul 28, 2016

I'm sorry, it is still conflicting.

Try git pull --rebase origin master (assuming the origin remote points to my repository, not your fork)

# Conflicts:
#	index.js
@SBoudrias SBoudrias merged commit d93e057 into SBoudrias:master Aug 14, 2016
@SBoudrias
Copy link
Owner

Thanks for sending this out!

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

2 participants