-
Notifications
You must be signed in to change notification settings - Fork 676
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
Use a case-insensitive find for files on Mac and Windows #6683
Conversation
} | ||
`) | ||
); | ||
expect(edits).toHaveLength(13); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this change, but is it related to this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, was just trying to get my builds green. Sorry :)
// can use paths as specified in the sln file. When these don't agree, on case-insensitive operating | ||
// systems, we have to be careful to match things up correctly. | ||
|
||
if (this.platformInfo.isLinux()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Roslyn have to do something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roslyn, like Razor, has basically the same idea: https://github.com/dotnet/roslyn/blob/main/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs#L675
// can use paths as specified in the sln file. When these don't agree, on case-insensitive operating | ||
// systems, we have to be careful to match things up correctly. | ||
|
||
if (this.platformInfo.isLinux()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will probably work most of the time, but its very possible to have either a case sensitive or insensitive file system on linux (and others). I'm not sure if there is a way to generally know in node though, other than checking for the existence of the path on disk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither Roslyn or Razor seem to do anything more sophisticated than this. See https://github.com/dotnet/roslyn/blob/main/src/Compilers/Core/Portable/FileSystem/PathUtilities.cs#L675
This low down in the code, I don't think file existence is something we can rely on either - we just want to compare two document paths. VS Code will happily keep around a document, and issue LSP requests for it, if it doesn't exist on disk. Likewise Roslyn will happily answer requests about it until it gets a didClose, etc.
Fixes #6644
The server side already does the same thing but we have a whole document manager here in TypeScript land, and I guess sometimes CPS uses the path in the sln file as a source of truth.
UPDATE: The razor formatting test failed in a weird spot, so I also changed how it asserts to hopefully make it more reliable.