-
Notifications
You must be signed in to change notification settings - Fork 215
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 issue where # in path causes the path to resolve incorrectly #750
Conversation
Our current testing means we can't test this stuff cross platform btw -- I can't create a file URI with a |
Still not sure what's causing PowerShell/vscode-powershell#1513 -- I can't reproduce it (even on a Mac) |
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.
Hoping this fixes allllll the Chinese & special character issues 😎 LGTM!
@@ -11,6 +11,10 @@ | |||
using System.Security; | |||
using System.Text; | |||
|
|||
#if CoreCLR | |||
using System.Runtime.InteropServices; |
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.
Is this used anywhere else? If not, maybe get rid of this ifdef and use the full name space below?
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.
Only used in the one line, but twice. We would need the full namespace on both RuntimeInformation.IsOSPlatform()
and OSPlatform.Windows
.
I just tried it, and I think the import is less cumbersome.
Fixes PowerShell/vscode-powershell#1518 in a second way!
We seemed to be double escaping URIs. I've changed the behaviour and added a battery of tests around
Workspace.ResolveFilePath()
(that we should add to).Now a URI like
file:///C:/foo/bar#x/baz/
will resolve toC:\foo\bar#x\baz\
and notC:\foo\bar
.