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

Added FromStr impl and tests for HostAndPort #264

Closed
wants to merge 1 commit into from

Conversation

@amikhalev
Copy link
Contributor

amikhalev commented Dec 28, 2016

This change is Reviewable

@SimonSapin
Copy link
Member

SimonSapin commented Dec 28, 2016

I’m somewhat hesitant to take this since there might be subtle variations of the exact parsing algorithm, and the URL Standard doesn’t specify one. The closest thing would be the setter of the host attribute of JavaScript URL objects. It’s not obvious to me whether the way that is specified is equivalent to the way you implemented it here.

Which brings the most important question: why do you want this? In what context would you use it? Is there a spec with a parsing algorithm or grammar?

@amikhalev
Copy link
Contributor Author

amikhalev commented Dec 28, 2016

The reason I want this is that I need to pass around a network address, but without the IP address resolved as it is with SocketAddr. The code that is using the address doesn't care about the scheme or path (so Url has too much info), but it needs the domain in certain cases for checking TLS certificates. This also happens to work well with with_default_port.

The only issue is that it is a bit unwieldy to construct a HostAndPort. This PR should make

let addr = HostAndPort {
    host: Host::Domain("localhost"), port: 80,
};

into

let addr: HostAndPort = "localhost:80".parse().unwrap();

Unfortunately, I do not know of any specification for parsing. But with my rudimentary knowledge of URLs, just chopping off everything after the last : and assuming that is the port seems like it should work in most cases. Although in cases like "[::1]" it may confuse :1] as the port part, but that would cause an error anyways.

Another option to this would a new or some other method which calls Host::parse, making it into

let addr = HostAndPort::new("localhost", 80);

but there would still have to be an unwrap somewhere for the result of Host::parse.

I'm not sure which way would be best. Your thoughts?

@SimonSapin
Copy link
Member

SimonSapin commented Dec 29, 2016

The only issue is that it is a bit unwieldy to construct a HostAndPort. This PR should make

let addr = HostAndPort {
    host: Host::Domain("localhost"), port: 80,
};

into

let addr: HostAndPort = "localhost:80".parse().unwrap();

It sounds like you just want a simple constructor, but Host::parse does much more than that. It will decode any percent-encoding, do Unicode normalization, encode to Punycode if there are non-ASCII characters, …

I’d recommend making your own type and a constructor method for them. Parsing the host part could be something along the lines of "try std::net::IpAddr::parse, and if that returns an error take the input as-is and consider it a domain".

But come to think of it, do you even need to parse IP addresses? Could you pass around (&str, u16) or (String, u16)? The std::net::ToSocketAddrs trait has an impl for (&str, u16) that does this parsing already, in case you eventually need to resolve this host to an IP address.

@dtolnay
Copy link

dtolnay commented May 12, 2017

Given that there is no spec with a parsing algorithm or grammar, we are not planning to provide this functionality in url right? Let's close the PR.

@SimonSapin
Copy link
Member

SimonSapin commented Jun 13, 2017

Closing for lack of response from @amikhalev about what exactly is desired.

@SimonSapin SimonSapin closed this Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.