-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: Force InitializeParams windows path drives to uppercase #14689
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
Conversation
Is there a way I can test this locally without having to wait for the weekly release? |
I can kick off a nightly after this has been merged, then you can test it in an hour or two I think |
b261086
to
140e1d7
Compare
@bors r+ |
fix: Force InitializeParams windows path drives to uppercase Should fix #14683 cc `@jyn514`
💔 Test failed - checks-actions |
140e1d7
to
8f0c78b
Compare
@bors r+ |
fix: Force InitializeParams windows path drives to uppercase Should fix #14683 cc `@jyn514`
💔 Test failed - checks-actions |
My god I hate the standard libraries path api so much @bors r+ |
8f0c78b
to
1edcefb
Compare
💡 This pull request was already approved, no need to approve it again.
|
☀️ Test successful - checks-actions |
format!("{}:", d.to_ascii_uppercase() as char) | ||
} | ||
Prefix::VerbatimDisk(d) => { | ||
format!(r"\\?\{}:\", d.to_ascii_uppercase() as char) |
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 apologize in advance for adding a comment in a PR long past, but should the trailing backslash be present in that UNC path prefix?
Asking because I'm fighting almost the exact same issue in my codebase and I'm not sure about this bit.
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.
Oh no, it should not I think. Though I don't think it makes a difference given how rust handles path appending
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.
Just checked - in this particular case it doesn't matter, though that's subject to change should the surrounding code get altered. I've opened #14799 to get it fixed.
Remove root component from patched Windows UNC path prefix As it shouldn't be there to begin with (per discussion in #14689).
Should fix #14683
cc @jyn514