Conversation
|
I agree with this change. But perhaps that big chunk of stuff being passed in the |
lwt/cohttp_lwt.ml
Outdated
There was a problem hiding this comment.
This unfortunately won't work, since Cohttp_lwt cannot depend on Unix -- only Cohttp_lwt_unix can. This information could possibly be encoded in the Cohttp.Connection.t though, and unpacked only in the relevant (Cohttp_lwt_unix or Mirage_http) backend.
There was a problem hiding this comment.
Another reason to perhaps making the first param to callback abstract? We could write our functor over it.
I did suggest Cohttp.Conncetion.t in the mailing list btw. I forgot why @dinosaure did not find satisfactory.
There was a problem hiding this comment.
Maybe I'm wrong took me to add this information and I have not spent a lot of time :) I'll think about it this weekend on how to use the abstract type Cohttp.Connection.t so that ships sockaddr in Cohttp_lwt_unix.
|
I think this is a better implementation but it is not perfect. I create a new Endpoint interface (same as IO model) for abstracting the sockaddr. The Connection module and its interface does not suit me and I think it should have used another (we can have a different ID but whose source is the same). |
|
@dinosaure could you rebase on top of master so I can try this branch out? Sorry, was too busy to review this carefully. |
|
Likewise, I'm just back from travelling and can look tomorrow morning.
|
Conflicts: _oasis _tags cohttp/cohttp.mlpack cohttp/cohttp.odocl lwt/cohttp_lwt.ml lwt/cohttp_lwt.mli setup.ml
|
I think, in regards new upstream/master, this PR is little outdated. This PR compile at home but I think that move Endpoint.S to S signature. I would look more precisely it tomorrow morning. EDIT: Don't forget to update mirage/ocaml-conduit#2 |
lwt/cohttp_lwt_unix_net.ml
Outdated
There was a problem hiding this comment.
Why is this module aliased here?
With the new endpoint module it's not clear what's the purpose of Connection. Seems better to get rid of old Connection and rename Endpoint to be Connection.
I notified this PR in mailing-list but I'm afraid it is unnoticed.