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

Fix documentation for file: URLs, and add an assist for malformed URLs #13272

Merged

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Oct 15, 2021

Fix documentation for file: URLs, and add an assist if the URL contains a hostname.

Fixes #13054.

[ci skip-build-wheels]

@stuhood stuhood enabled auto-merge (squash) October 15, 2021 23:00
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Sweet, thanks!

"work with proxies or for access via the filesystem through a file:// URL.\n\nUse "
"`{version}` to have the value from --version substituted, and `{platform}` to "
"work with proxies or for access via the filesystem through a `file:$path` URL (e.g. "
"`file:/this/is/absolute` or `file:this/is/relative`).\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

Is it not common for url like paths such as these to have a file:// prefix in front of the path?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

It is. But the impact of that syntax in this case is that the first token is parsed as a hostname (which apparently is supported as part of the standard: see the loc%61lhost example in https://url.spec.whatwg.org/#urls). I think that what these are instead is path-absolute-URL strings.

Copy link
Member

Choose a reason for hiding this comment

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

OK, haven't seen that page before. And it was anything but clear. However, from my understanding the general syntax is (simplified) scheme://<server>/<path>, and <server> may be the empty string, so for file schemes, this translates to: file:///this/is/absolute rather than file:/this/is/absolute.
I didn't find any reference to a scheme followed by :/ (single slash only) in there..

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

See https://url.spec.whatwg.org/#relative-url-string:

"file"
..
a path-absolute-URL string if base URL’s host is an empty host

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/document-file-urls-and-add-assist branch from 5972809 to d0c116e Compare October 20, 2021 21:16
@stuhood
Copy link
Sponsor Member Author

stuhood commented Oct 20, 2021

I was wrong about relative paths being supported: they aren't without supplying a base URL, which doesn't seem worth it in this case. Have cleaned up the assist and the docs to account for that.

@stuhood stuhood merged commit 7d0879c into pantsbuild:main Oct 20, 2021
@stuhood stuhood deleted the stuhood/document-file-urls-and-add-assist branch October 21, 2021 15:57
Comment on lines +109 to +110
url.path(),
url.path(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so excited for Rust to have f-strings...

Copy link
Contributor

@tdyas tdyas Oct 21, 2021

Choose a reason for hiding this comment

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

That's what format! is. This code should have just used named substitutions:

format!("The file Url `{url}` has a host component. Instead, use `file:$path`, \
        which in this case might be either `file:{host}{path}` or `file:{path}`.",
        url = url,
        host = host,
        path = url.path(),
)

https://doc.rust-lang.org/std/macro.format.html

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

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.

Document/fix quirks for file:// URLs for tool downloads
5 participants