Skip to content
This repository was archived by the owner on Oct 2, 2021. It is now read-only.

Canonicalize win32 paths to lowercase#342

Merged
roblourens merged 5 commits intomicrosoft:masterfrom
digeff:improvements_canonicalize_win32
Jul 20, 2018
Merged

Canonicalize win32 paths to lowercase#342
roblourens merged 5 commits intomicrosoft:masterfrom
digeff:improvements_canonicalize_win32

Conversation

@digeff
Copy link
Contributor

@digeff digeff commented Jul 18, 2018

Canonicalize win32 paths to lowercase

mrcrane
mrcrane reviewed Jul 18, 2018
View reviewed changes
src/utils.ts Outdated
: urlOrPath;
}

export function isFilePath(candidate: string): boolean {
Copy link
Contributor

@mrcrane mrcrane Jul 18, 2018

Choose a reason for hiding this comment

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

This will only match Windows-style paths. If it won't match unix-style paths, can you give the method a more specific name? Does it need to be exported?

Copy link
Contributor Author

@digeff digeff Jul 18, 2018

Choose a reason for hiding this comment

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

Changed to isWindowsFilePath

@digeff digeff force-pushed the improvements_canonicalize_win32 branch from 4bfa1f2 to bd897e0 Compare July 18, 2018 23:46
@digeff digeff force-pushed the improvements_canonicalize_win32 branch from bd6d219 to 9e74861 Compare July 19, 2018 00:07
@auchenberg
Copy link
Contributor

auchenberg commented Jul 19, 2018 •
edited
Loading

@digeff What's the problem being solved here? Please provide some context for the reviewers here. Should this happen on all platforms? etc?

@digeff
Copy link
Contributor Author

digeff commented Jul 19, 2018

@auchenberg This is Windows only
We get paths from different places, and they can have different cases even if they refer to the same file. We need to make sure that the code doesn't consider the case when comparing file paths.
In VS we get the webRoot from the ProjectSystem, the file paths from the Debugger, and other file paths from Chrome or Node. In node the case depends on the case used on the require('myFile') statement. There is no easy way to make sure that all those components use the same case, so the solution seems to be to make sure that the comparison is always case insensitive.

@roblourens roblourens merged commit 9e9eabc into microsoft:master Jul 20, 2018
@roblourens
Copy link
Member

roblourens commented Jul 20, 2018

@digeff Could you write a test that captures the case that was missed, and prompted this PR? I think you said it was for the webroot?

@digeff digeff deleted the improvements_canonicalize_win32 branch July 20, 2018 17:28
@roblourens roblourens added this to the July 2018 milestone Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@mrcrane mrcrane mrcrane left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@roblourens roblourens

Labels

None yet

Projects

None yet

Milestone

July 2018

Development

Successfully merging this pull request may close these issues.

4 participants

@digeff @auchenberg @roblourens @mrcrane