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 implementation for ToSocketAddrs #533

Closed
wants to merge 1 commit into from

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Jul 30, 2019

I thought of this while writing up #532.

url::Url's ToSocketAddrs implementation was dropped because of its large
API surface. Restore it with a simpler implementation that does not
introduce as much API surface. The upside is that the HostAndPort and
SocketAddrs helper types are no longer necessary. The downside is that
if the URL specifies an IP address, the ToSocketAddrs implementation has
to parse the IP address from a string again.


This change is Reviewable

url::Url's ToSocketAddrs implementation was dropped because of its large
API surface. Restore it with a simpler implementation that does not
introduce as much API surface. The upside is that the HostAndPort and
SocketAddrs helper types are no longer necessary. The downside is that
if the URL specifies an IP address, the ToSocketAddrs implementation has
to parse the IP address from a string again.
@SimonSapin
Copy link
Member

Alright, #532 and this PR make a convincing case that this functionality should be present. I had some comments on API details, and writing another PR felt easier. Closing in favor of #535, please comment there about its proposed API. Thanks anyway for submitting this PR!

@SimonSapin SimonSapin closed this Jul 31, 2019
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 -->
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.

2 participants