fix: get rid of flush #1052
fix: get rid of flush #1052
Conversation
cohttp/src/response.ml
Outdated
| let make ?(version = `HTTP_1_1) ?(status = `OK) ?(flush = false) | ||
| ?(encoding = Transfer.Chunked) ?(headers = Header.init ()) () = | ||
| ignore flush; |
There was a problem hiding this comment.
| let make ?(version = `HTTP_1_1) ?(status = `OK) ?(flush = false) | |
| ?(encoding = Transfer.Chunked) ?(headers = Header.init ()) () = | |
| ignore flush; | |
| let make ?(version = `HTTP_1_1) ?(status = `OK) | |
| ?(encoding = Transfer.Chunked) ?(headers = Header.init ()) () = |
Why not dropping flush also from here?
There was a problem hiding this comment.
I think this would be a bigger breaking change. Since this make function has been around for a long time in many released versions. I'm honestly happy to get rid of it as well though if you prefer.
There was a problem hiding this comment.
I think at this point why not. Removing the flush field is already breaking, we may as well do it once and for all
There was a problem hiding this comment.
If we're talking about the 6.0 major version upgrade, I think the point is precisely to allow API breakage? One has to do some cleanup at some point, and major versions are the only opportunity we have of doing so.
rgrinberg
force-pushed
the
ps/rr/fix__get_rid_of_flush
branch
from
300b29f to
651ef43
Compare
We haven't yet released [Http] so it's ok to change things. It doesn't seem right to release the library with a deprecated feature. It's easy enough to just drop this field in [Http] Signed-off-by: Rudi Grinberg <[email protected]> <!-- ps-id: adbceded-c111-4ce6-8083-96c5a8a7ca26 -->
rgrinberg
force-pushed
the
ps/rr/fix__get_rid_of_flush
branch
from
651ef43 to
6101a53
Compare
|
@mefyl do you want to have a look? |
|
LGTM ! |
Cohttp 6 introduced a breaking change to the `Response` API dropping `flush` in mirage/ocaml-cohttp#1052 This updates our usage to be compatible.
Cohttp 6 introduced a breaking change to the `Response` API dropping `flush` in mirage/ocaml-cohttp#1052 This updates our usage to be compatible.
We haven't yet released [Http] so it's ok to change things. It doesn't seem right to release the library with a deprecated feature.
It's easy enough to just drop this field in [Http]
Signed-off-by: Rudi Grinberg [email protected]