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

proposal: net/url: add FromFilePath and ToFilePath #32456

Open
bcmills opened this issue Jun 5, 2019 · 4 comments
Open

proposal: net/url: add FromFilePath and ToFilePath #32456

bcmills opened this issue Jun 5, 2019 · 4 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 5, 2019

In the course of investigating #27698, I learned that the conversion between URLs and file paths on Windows is rather non-trivial. (See also #6027.)

According to https://blogs.msdn.microsoft.com/ie/2006/12/06/file-uris-in-windows/, a file path beginning with a UNC prefix should use the UNC hostname as the “host” part of the URL, with only two leading slashes, whereas a file path beginning with a drive letter should omit the “host” part and prepend a slash to the remainder of the path.

Once you know those rules, the implementation in each direction is only about ten lines of code, but it's easy to get wrong (for example, by neglecting the possibility of a UNC prefix) if you haven't thought about it in depth.

I propose that we add two functions to the net/url package to make these cases more discoverable, with the following signatures:

// ToFilePath converts a URL with scheme "file" to an absolute file path.
// It returns a non-nil error if the URL does not have the scheme "file" or
// the resulting path is not well-formed.
func ToFilePath(u *URL) (string, error) {
	[…]
}

// FromFilePath converts an absolute file path to a URL.
func FromFilePath(path string) (*URL, error) {
	[…]
}

CC @alexbrainman

@gopherbot gopherbot added this to the Proposal milestone Jun 5, 2019
@gopherbot gopherbot added the Proposal label Jun 5, 2019
@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Jun 6, 2019

There is at least one place where we have already gotten this wrong in the standard library:

if !browser.Open("file://" + out.Name()) {

@mvdan

This comment has been minimized.

Copy link
Member

@mvdan mvdan commented Jun 7, 2019

I assume most uses will be string-based - is there a reason why you've designed the API around url.URL? It's easy to use url.Parse and url.URL.String to use the proposed API with strings, but the same could be said about using a string-based API with URLs.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Jun 7, 2019

@mvdan, two reasons:

  1. Using url adds type-safety at the call site. (It's much less likely that someone will accidentally confuse a URL for a file-path — or vice-versa — if they have different types.)
  2. The subtle parts of converting Windows paths to file:// URLs involve the Host field of the URL. It's much easier to express those subtle parts in terms of the Host and Path fields, and seems easier for users to diagnose unexpected results when they can clearly see how the path was split into a Host and Path (rather than counting slashes).
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 13, 2019

I think there's enough subtlety in the implementation here - even though the docs sound simple - that this is worth a design doc laying out the issues that an implementation will have to address. There's no rush on this, @bcmills, but I think that's the next step.

Will mark it on-hold for design doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.