Skip to content
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(convert/pathToUri): Improve the encoding of non-alphanumeric paths other than the file protocol #183

Merged
merged 4 commits into from
Aug 6, 2021

Conversation

ayame113
Copy link
Contributor

@ayame113 ayame113 commented Aug 5, 2021

resolve ayame113/atom-ide-deno#74

There is a difference in the conversion process between Convert.uriToPath and Convert.pathToUri.

function (arguments -> retuen value)
uriToPath() https://foo.bar/%40->https://foo.bar/%40
https://foo.bar/%2540->https://foo.bar/%2540
pathToUri() https://foo.bar/%40->https://foo.bar/%2540

The decoding process of urls other than file: was changed by 6f1e149, but it seems that the encoding process was not changed.
This PR modifies the encoding process for paths other than file: .

@ayame113
Copy link
Contributor Author

ayame113 commented Aug 5, 2021

Oops, the test failed, so I'll fix it. I fixed. This is ready for review.

Comment on lines 52 to 58
it("does not encode Windows drive specifiers", () => {
expect(Convert.pathToUri("d:\\ee\\ff.txt")).toBe("file:///d:/ee/ff.txt")
// This test only succeeds on windows. (Because of the difference in the processing method of drive characters)
// However, it is enough to test the windows drive character only on windows.
if (process.platform === "win32") {
expect(Convert.pathToUri("d:\\ee\\ff.txt")).toBe("file:///d:/ee/ff.txt")
}
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new URL("d:\\ee\\ff.txt", "file://") seems to return different results for windows and others. I changed this because I thought it would be enough if the test on the drive character was successful only on windows.

@aminya aminya added the bug Something isn't working label Aug 6, 2021
@aminya aminya merged commit 5c6021d into atom-community:master Aug 6, 2021
@github-actions
Copy link

github-actions bot commented Aug 6, 2021

🎉 This PR is included in version 1.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in virtual document with @ in path
3 participants