Skip to content

Redoing PR for Travis#133

Closed
rgrinberg wants to merge 1 commit into
mirage:masterfrom
rgrinberg:pr/130
Closed

Redoing PR for Travis#133
rgrinberg wants to merge 1 commit into
mirage:masterfrom
rgrinberg:pr/130

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg
Copy link
Copy Markdown
Member Author

Why do we have the ipaddr to string commit? Seems like it's breaking compatibility.

@rgrinberg rgrinberg force-pushed the pr/130 branch 2 times, most recently from 794ee1a to 82c4032 Compare June 12, 2016 06:31
@vbmithr
Copy link
Copy Markdown
Member

vbmithr commented Jun 12, 2016

The ability to use hostnames (that are resolved by Async) was negated by the old interface. The Ipaddr.t provided was anyway converted as a string. In practice, if you want to connect to an URL in text form, how do you get an IP address from it with Ipaddr.t with Async?

This is indeed breaking API change but I think this is better. What do you think? If you don't, do you know any straightforward way to get an Ipaddr.t from a hostname (I'm not using Lwt) ?

@rgrinberg
Copy link
Copy Markdown
Member Author

I see. That's indeed annoying. What about introducing a new set of
constructors that allow people to specify the host names directly?

Since this is a breaking change, I'd like to hear what other maintainers
think.

@dsheets @avsm @samoht

On 06/12, Vincent Bernardoff wrote:

The ability to use hostnames (that are resolved by Async) was negated by the old interface. The Ipaddr.t provided was anyway converted as a string. In practice, if you want to connect to an URL in text form, how do you get an IP address from it with Ipaddr.t with Async?

This is indeed breaking API change but I think this is better. What do you think? If you don't, do you know any straightforward way to get an Ipaddr.t from a hostname (I'm not using Lwt) ?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#133 (comment)

@rgrinberg
Copy link
Copy Markdown
Member Author

@vbmithr OK I've thought over this more and without more feedback I'm in favor of this change.

A couple of things still bug me however:

  1. Ipaddr.t isn't used in any of the constructors. I suppose that's alright.
  2. Our interface still isn't exposing the "async way" of constructing an endpoint which is the Tcp.where_to_connect type. Does it make sense to expose that in another constructor as well perhaps?

@vbmithr
Copy link
Copy Markdown
Member

vbmithr commented Sep 1, 2016

@rgrinberg Yes definitely we need the same constructor as in Async. I would even remove the existing one to leave only this one. For the Ipaddr.t not used, I don't see why it would be a problem.

@samoht samoht force-pushed the master branch 3 times, most recently from 146d013 to 84179f7 Compare November 6, 2016 17:11
@dinosaure dinosaure mentioned this pull request Sep 29, 2020
@dinosaure
Copy link
Copy Markdown
Member

Due to #311, this PR will be close. I think we fixed on #311 what we tried to do in this PR. Thanks!

@dinosaure dinosaure closed this Sep 29, 2020
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