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

Expose write failures in flush #3

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

ada2k
Copy link
Contributor

@ada2k ada2k commented Jul 26, 2024

See inhabitedtype/httpaf#217 and https://github.com/ada2k/httpaf/tree/expose-write-failures-in-flush

I've changed this to add back in flush: t -> (unit -> unit) -> unit for old code and code that only expects to flush once and so doesn't care about the result.

The original PR changes write_ functions to be guarded behind an if statement, and do nothing instead of raising if the user tries to write to a closed Faraday.t so that this can be properly handled in flush_with_reason. I've removed tests that expect for it to raise.

Sidenote: I also have a patch which integrates httpun-types for greater interop between both ocaml-h2 and httpun. LMK if this would be appreciated by h1 :)

@dinosaure
Copy link
Contributor

If you can keep authors of commits (@dpatti and @dhouse-js - a Co-authored will be enough I think), it will be super nice. About httpun, feel free to make a PR and I can review then.

@ada2k ada2k force-pushed the expose-write-failures-in-flush branch from ea6ed49 to b325ffe Compare July 31, 2024 18:30
Completes merge of inhabitedtype/httpaf#217

Co-authored-by: David House <[email protected]>
Co-authored-by: Doug Patti <[email protected]>
@ada2k ada2k force-pushed the expose-write-failures-in-flush branch from b325ffe to 2d95e23 Compare July 31, 2024 18:30
@ada2k
Copy link
Contributor Author

ada2k commented Jul 31, 2024

All done

@dinosaure dinosaure merged commit ed3b95c into robur-coop:master Aug 19, 2024
8 checks passed
@dinosaure
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants