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

Improve URI Parsing #582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve URI Parsing #582

wants to merge 1 commit into from

Conversation

vladaviedov
Copy link

@vladaviedov vladaviedov commented Jun 23, 2024

Go to Definition throws an exception when a JAR contains a plus sign in its name (as described in #326). parseURI calls URLDecoder.decode on its input, but if the input is not encoded, it converts the plus to a space.

I tried to remove the decoding and properly encode the file path for later usage, which seems to be working for me in both neovim and vscode, but I have no idea how to test the "invalid percent encoding for colons".

@github-actions github-actions bot added the code quality Refactoring, tests etc. label Jun 23, 2024
Copy link
Owner

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Thanks, I'll take a closer look once I find some time. Just a quick note below

@@ -14,7 +14,7 @@ import java.nio.file.Paths
import java.util.zip.ZipFile

fun URI.toKlsURI(): KlsURI? = when (scheme) {
"kls" -> KlsURI(URI("kls:${schemeSpecificPart.replace(" ", "%20")}"))
"kls" -> KlsURI(URI("kls:${schemeSpecificPart}"))
Copy link
Owner

Choose a reason for hiding this comment

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

We'll have to be extra careful here though not to regress any popular client (including VSCode on all major platforms). I remember URL encoding being a tricky subject since different clients often handle percent encodings in subtly different ways.

Copy link
Author

Choose a reason for hiding this comment

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

Might Windows drive letters be the issue? It's the only part of the spec that seems to mention URI inconsistencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Refactoring, tests etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants