windows: add a separate local_shorthand_path test#15307
Conversation
puffnfresh
force-pushed
the
windows-local-shorthand
branch
2 times, most recently
from
60a52cf to
d796754
Compare
| .variant = | ||
| StoreReference::Specified{ | ||
| .scheme = "local", | ||
| .authority = "C:\\foo\\bar\\baz", |
There was a problem hiding this comment.
Not sure this is actually right. Since store references are in fact URLs this would be local://C:/foo/bar/baz" - which has the hostname C with an empty port... (or worse yet, no forward slashes mean that it won't parse at all as a URL)
Let's not do this. If anything, we should take the same approach as with file:// URL spec - i.e. local:///C:/foo/bar/baz (note the first slash in path and all forward slashes).
There was a problem hiding this comment.
That's a pretty good argument. UrlCreateFromPath gives things like file:///c:/path/to/the%20file.txt, which makes sense and should build upon your PR.
|
Also we should probably build proper fixes upon #15280 |
Ericson2314
force-pushed
the
windows-local-shorthand
branch
from
857a932 to
4d5df7d
Compare
Ericson2314
left a comment
Ericson2314
left a comment
There was a problem hiding this comment.
@xokdvium I think this is good now? It is not atop your PR, but it should be very compatible with it
| Specified{ | ||
| .scheme = "local", | ||
| .authority = absPath(baseURI), | ||
| .authority = encodeUrlPath(pathToUrlPath(absPath(std::filesystem::path{baseURI}))), |
There was a problem hiding this comment.
Without the pct-encoding fix PR this isn't exactly correct, but overall this seems more correct tbh.
There was a problem hiding this comment.
Yeah I think it will nicely work with your PR.
This missing URL functionality allow us to properly fix the path shorthand for local store URLs test case.
Ericson2314
force-pushed
the
windows-local-shorthand
branch
from
4d5df7d to
3f419cf
Compare
windows: add a separate local_shorthand_path test
3 participants
@Ericson2314