Skip to content
This repository was archived by the owner on Mar 13, 2018. It is now read-only.

replace backslashes to support windows filesystem#217

Open
mcarey1590 wants to merge 2 commits intogooglearchive:masterfrom
mcarey1590:master
Open

replace backslashes to support windows filesystem#217
mcarey1590 wants to merge 2 commits intogooglearchive:masterfrom
mcarey1590:master

Conversation

@mcarey1590
Copy link

No description provided.

@mcarey1590 mcarey1590 changed the title replace backslashes to support windows filesystem #215 replace backslashes to support windows filesystem Nov 17, 2015
@mbleigh
Copy link
Contributor

mbleigh commented Nov 17, 2015

I think the solution here should be to split on path.sep instead of replacing \\

@mcarey1590
Copy link
Author

we still have to replace the \ because we are replacing the base path with an empty string before splitting

@blasten
Copy link
Contributor

blasten commented Dec 1, 2015

👍 It should use path.sep https://github.com/jinder/path/blob/master/path.js#L414

Copy link
Contributor

Choose a reason for hiding this comment

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

split(path.sep)

@addyosmani
Copy link
Contributor

Friendly ping @mcarey1590 on switching to split(path.sep).

@mcarey1590
Copy link
Author

the issue I see with switching to split(path.sep) is that replacing cwd has to happen before the split, and the srcPath with always use forward slashes. On a windows machine the srcPath always had forward slashes where the cwd always used backslashes. Splitting srcPath on path.sep would not work on a windows environment.

@arthurevans
Copy link
Contributor

How about:

var relativeSrcPath = path.relative(process.cwd(), srcPath).split(path.sep);

I think path.relative will correctly normalize windows path names (i.e., will correctly recognize foo\\bar/blech as relative to foo/bar).

@mcarey1590
Copy link
Author

I tried Arthur's suggestion and window's path names were normalized correctly. This change has been added to pull request

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants