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

warp: Use more meaningful types instead of Bool #998

Closed
wants to merge 1 commit into from

Conversation

sol
Copy link
Contributor

@sol sol commented Aug 1, 2024

The overuse of booleans for all kinds of things makes it somewhat hard to follow the code that implements HTTP/1.1 keep-alive. This is a step towards trying to improve the situation.

@@ -50,11 +51,13 @@ import Network.Wai.Handler.Warp.Settings (

----------------------------------------------------------------

-- | first request on this connection?
data FirstRequest = FirstRequest | SubsequentRequest

-- | Receiving a HTTP request from 'Connection' and parsing its header
-- to create 'Request'.
recvRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, this is re-exported from Network.Wai.Handler.Warp.Internal. @kazu-yamamoto @Vlix what are the stability guarantees for that module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change Internal at anytime without major version up.

@kazu-yamamoto kazu-yamamoto self-requested a review August 5, 2024 02:24
@kazu-yamamoto
Copy link
Contributor

kazu-yamamoto commented Aug 5, 2024

I have fixed CI and rebase this PR onto master.
CI failed again.
I fixed Bench but still failing.

https://github.com/kazu-yamamoto/wai/tree/ci
https://github.com/kazu-yamamoto/wai/actions/runs/10244402661/job/28337330217

@Vlix
Copy link
Contributor

Vlix commented Aug 5, 2024

I'll take a look at it tomorrow 👍

kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this pull request Aug 6, 2024
@kazu-yamamoto
Copy link
Contributor

With my refreshed head, it is easy to fix this.
I have merged this PR after rebase with my fix.
Thank you for your contribution!

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