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

Cohttp 0.18.0 compatibility #186

Merged
merged 1 commit into from
Jun 5, 2015
Merged

Cohttp 0.18.0 compatibility #186

merged 1 commit into from
Jun 5, 2015

Conversation

rgrinberg
Copy link
Contributor

Use Request/Response directly from Cohttp base module. The lwt specific
versions have been removed in cohttp 0.18.0 since they were the same
anyway.

I think this should make ketrew compatible with 0.17.0 and 0.18.0

Review on Reviewable

Use Request/Response directly from Cohttp base module. The lwt specific
versions have been removed in cohttp 0.18.0 since they were the same
anyway.
@smondet
Copy link
Member

smondet commented Jun 5, 2015

I did something similar a few minutes ago :)

805e6b7

I used Cohttp_lwt_unix in case the types change in the future
https://github.com/mirage/ocaml-cohttp/blob/master/lwt/cohttp_lwt.mli#L40
(e.g. for Body.t they are different for a good reason:
https://github.com/mirage/ocaml-cohttp/blob/master/lwt/cohttp_lwt.mli#L40).

@rgrinberg
Copy link
Contributor Author

They are different for a good reason but that's another difference we're trying to get rid of :)

We're trying to get rid of the backend specific request/response modules because all they do is expose unnecessary implementation details. Just a warning not to rely on the backend specific bits request/response because they're mostly there to implement the cohttp server/client in lwt/async.

This is why I recommend just to use Cohttp.{Request,Response}

@smondet
Copy link
Member

smondet commented Jun 5, 2015

OK, I'll merge yours :)

BTW for Body.t, the special case of a “stream” is easy to abstract; a stream can just be a unit -> string option IO.t function.

@smondet smondet merged commit 71b51b2 into hammerlab:master Jun 5, 2015
smondet added a commit that referenced this pull request Jun 5, 2015
@rgrinberg
Copy link
Contributor Author

Yeah i have something like that in the works for body. The demand for it has proven to be pretty low though because streams are discouraged in async anyway.

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.

None yet

2 participants