Handle colons in file names when producing SemanticDB #15863
Conversation
|
Apparently in CI it does not pass all existing tests. I'm not sure how to interpret the job failure messages though. Can someone help me out? |
something changed in the output of semanticdb - you can test if this was intentional by running which will update some "expect" files, (used for regression testing semanticdb output) and then comparing the diff with git. |
kierendavies
force-pushed
the
semanticdb-urisyntaxexception
branch
from
09ca3f2
to
11cfdae
Compare
|
Thank you @bishabosha. It was a bug, which I've fixed. |
| // treating the part as an absolute path, and then remove it afterwards. | ||
| val uriParts = | ||
| for part <- path.asScala | ||
| yield new java.net.URI(null, null, "/" + part.toString, null).toString().stripPrefix("/") | ||
| uriParts.mkString("/") |
There was a problem hiding this comment.
would it have the same result if you don't strip the prefix from each individual part, but instead do uriParts.mkString("").stripPrefix("/")
There was a problem hiding this comment.
Good idea - I completely missed that. I think it'll have the same result.
kierendavies
force-pushed
the
semanticdb-urisyntaxexception
branch
from
11cfdae
to
53e1adc
Compare
|
hopefully CI passes, thank you! |
| uriParts.mkString("/") | ||
| // To prevent colon `:` from being treated as a scheme separator, prepend a slash `/` to each part to trick the URI | ||
| // parser into treating it as an absolute path, and then strip the spurious leading slash from the final result. | ||
| val uriParts = for part <- path.asScala yield new java.net.URI(null, null, "/" + part.toString, null) |
There was a problem hiding this comment.
looks like this URI constructor is just not the right tool for the job...
There was a problem hiding this comment.
It's a poor fit for sure, but sadly the Java standard library doesn't provide anything better. I considered copy-pasting the OpenJDK private method on java.net.URI that does the quoting, but it's, uh, very Java. And then there's also license compatibility.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Fixes #15860
This solution feels pretty hacky to me, but it passes all existing tests... 🤷