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

Restore DNS resolution functionality from url 1.x #535

Merged
merged 2 commits into from
Aug 5, 2019
Merged

Conversation

SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jul 31, 2019

This is an alternative to #533.

I feel that an inherent method is more appropriate than an impl of the ToSocketAddrs trait: it is not the URL as a whole that is converted, only parts of it.


This change is Reviewable

@SimonSapin
Copy link
Member Author

r=nox per IRC discussion.

@benesch what do you think of this API?

@benesch
Copy link
Contributor

benesch commented Aug 1, 2019

I feel that an inherent method is more appropriate than an impl of the ToSocketAddrs trait: it is not the URL as a whole that is converted, only parts of it.

Perhaps you have a more nuanced understanding of what it means to implement ToSocketAddrs than I do. The Rust docs say ToSocketAddrs is "a trait for objects which can be converted or resolved to one or more SocketAddr values", which seems applicable enough to Url. To be honest, I'm not sure I see how a method named socket_addrs on Url is better at conveying that only part of the URL is used in the conversion.

I'd also like to point out that, in #533, implementing ToSocketAddrs on Url itself isn't strictly required. We could just restore a slightly tweaked with_default_port method instead:

+    pub fn with_default_port<F>(&self, f: F) -> io::Result<(&str, u16)>
+    where
+        F: FnOnce(&Url) -> Result<u16, ()>,
+    {
+        Ok((self
+                .host_str()
+                .ok_or(())
+                .or_else(|()| io_error("URL has no host"))?,
+            self
+                .port_or_known_default()
+                .ok_or(())
+                .or_else(|()| f(self))
+                .or_else(|()| io_error("URL has no port number"))?,
+        ))
+    }
+

As I mentioned in #533, this approach has the definite defect that it may cause an already-parsed IP address to be reparsed, a defect which is not present in this PR. (Although the extra vec allocation might mean that it's a wash.) But it has the upside that a lot of code written against 1.0 that used with_default_port would just work with this implementation with no code changes.

But! I'm happy enough with the approach in this PR! I'm quite glad I was able to convince you to restore this functionality at all. :)

@SimonSapin
Copy link
Member Author

It’s a bit hand-wavy, I interpret “objects which can be converted or resolved to” as the object is something that can be converted, whereas I view URLs as having components that can be converted. They also have other components that are not relevant to that conversion.

@bors-servo r=nox

@bors-servo
Copy link
Contributor

📌 Commit 2f199ff has been approved by nox

@bors-servo
Copy link
Contributor

⌛ Testing commit 2f199ff with merge a93e8cb...

bors-servo pushed a commit that referenced this pull request Aug 5, 2019
Restore DNS resolution functionality from url 1.x

This is an alternative to #533.

I feel that an inherent method is more appropriate than an impl of the `ToSocketAddrs` trait: it is not the URL as a whole that is converted, only parts of it.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/535)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: nox
Pushing a93e8cb to master...

@bors-servo bors-servo merged commit 2f199ff into master Aug 5, 2019
@SimonSapin SimonSapin deleted the socket_addrs branch August 5, 2019 15:01
@SimonSapin
Copy link
Member Author

This is in https://crates.io/crates/url/2.1.0

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.

3 participants