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

Fixed URL decode on Windows #2357

Closed
wants to merge 1 commit into from
Closed

Fixed URL decode on Windows #2357

wants to merge 1 commit into from

Conversation

roovio
Copy link

@roovio roovio commented Mar 13, 2019

l:encoded_path does not contain ':' so regexp match will fail and substitute() never called. The end result thus would still be '/C:/foo/bar' that the initial code presumably wanted to fix.

@w0rp
Copy link
Member

w0rp commented Mar 13, 2019

I don't think this is right. I think file:///C%3A/foo/bar/baz.tst should be converted to /C:/foo/bar/baz.tst. Otherwise it's not possible to represent /C:/foo in Linux.

@w0rp w0rp closed this Mar 13, 2019
@roovio
Copy link
Author

roovio commented Mar 13, 2019

At the moment GoToDefinition is broken on Windows because /C:/foo/bar is not a valid file (vim creates an empty buffer).
Judging by the comment the initial code was there to exactly adress this issue:

    " If the path is like /C:/foo/bar, it should be C:\foo\bar instead.

If we want to have equally supported /C:/foo/bar on Linux and C:\foo\bar on Windows then the URL decoder might need to have additional platform checks.

@w0rp
Copy link
Member

w0rp commented Mar 13, 2019

The URI received should be file:///C:/foo/bar/baz.tst, which will be translated to C:\foo\bar\baz.tst. The colon in the URI must not be encoded.

@roovio
Copy link
Author

roovio commented Mar 13, 2019

With ccls I am receiving file:///C%3A/Users/....
Checked by hacking into ale#definition#HandleLSPResponse l:item.uri

@w0rp
Copy link
Member

w0rp commented Mar 13, 2019

Open a bug for that project and get them to output URIs with the colon in them. They shouldn't be encoding the colon in the drive letter part of the path.

@roovio
Copy link
Author

roovio commented Mar 13, 2019

Ok thanks

@w0rp
Copy link
Member

w0rp commented Mar 13, 2019

On second thought, I'll just accept it and add a platform check. They should still output a colon without encoding it.

@w0rp
Copy link
Member

w0rp commented Mar 13, 2019

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants