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

Don't escape semicolons after drive letters #63

Merged
merged 1 commit into from
Dec 22, 2017

Conversation

nponeccop
Copy link
Contributor

The RFC says that semicolons may be used for special purposes in path parts of urls, and in this case no escaping is allowed. And this is consistent with Microsoft samples.

See haskell/haskell-ide-engine#329 for discussion
See autozimu/LanguageClient-neovim#144 for normative references and Microsoft examples of typical errors in URLs

The RFC says that semicolons may be used for special purposes in path parts of urls, and in this case no escaping is allowed. And this is consistent with Microsoft samples.

See haskell/haskell-ide-engine#329 for discussion
See autozimu/LanguageClient-neovim#144 for normative references and Microsoft examples of typical errors in URLs
@nponeccop
Copy link
Contributor Author

@alanz ping :)

@alanz
Copy link
Collaborator

alanz commented Dec 10, 2017

@wz1000 is currently reworking things, that includes bringing in a different filepath in ghc-mod. Once that is done we can confirm whether this issue still exists.

@alanz
Copy link
Collaborator

alanz commented Dec 10, 2017

And raeading more closely, this is not the issue I thought it was :(

@nponeccop
Copy link
Contributor Author

So what are we going to do? Correct per specification url escaping is a good thing as we deal with them all the time.

@alanz
Copy link
Collaborator

alanz commented Dec 10, 2017

We will fix it, I read the backscroll and got confused about what precisely it was.

But I first want the @wz1000 async stuff to land, as it touches a lot of the repos involved in this one, and I don't wont a tricky merge again.

The best will be to use an actual URI type, so that escaping gets handled as per the spec.

@nponeccop
Copy link
Contributor Author

nponeccop commented Dec 10, 2017

Oh, I see. We all hate tricky merges :)

The best will be to use an actual URI type

Sure. This was sort of a hot fix as many people reported the problem.

BTW in this case a totally correct solution is tricky. The specification says that : must not be escaped if it's used for a special purpose defined by the scheme. So basically per spec we should not escape if the URL is from Windows and this is the special semicolon after the drive letter, and should escape in other cases.

@nponeccop
Copy link
Contributor Author

nponeccop commented Dec 17, 2017

@alanz Can we merge it now?

@soylens at haskell/haskell-ide-engine#396 seems to report that %3a still happens after the @wz1000 merge.

@k0aaa
Copy link

k0aaa commented Dec 17, 2017

Have you not been experiencing the %3a since @wz1000 's commit?

@nponeccop
Copy link
Contributor Author

I haven't tested hie on windows after the commit yet.

@wz1000 wz1000 merged commit 4462fa7 into haskell:master Dec 22, 2017
lukel97 added a commit that referenced this pull request Feb 27, 2021
Part of the work towards #63.
The session will now keep track of the capabilities that are registered
and unregister them when requests come in from the server.
openDoc' has been removed and replaced with createDoc.
createDoc will send out workspace/didChangeWatchedFiles notifications
whenever the server registers for it.
lukel97 added a commit that referenced this pull request Feb 27, 2021
Part of the work towards bubba/lsp-test/#63.
The session will now keep track of the capabilities that are registered
and unregister them when requests come in from the server.
openDoc' has been removed and replaced with createDoc.
createDoc will send out workspace/didChangeWatchedFiles notifications
whenever the server registers for it.
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.

4 participants