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

refactor: make Response module pure #2971

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

develop7
Copy link
Collaborator

@develop7 develop7 commented Sep 27, 2023

Part of #2771

created per the suggestion at #2964 (comment)

@develop7 develop7 force-pushed the feat-2771-response_pure branch from f79b359 to b71011f Compare September 27, 2023 14:15
@develop7 develop7 marked this pull request as ready for review September 27, 2023 14:17
@develop7
Copy link
Collaborator Author

@steve-chavez done

@steve-chavez
Copy link
Member

I expected the Wai module usage to go away from Response after this change, but it's still in some places:

addRetryHint :: Int -> Wai.Response -> Wai.Response
addRetryHint delay response = do
let h = ("Retry-After", BS.pack $ show delay)
Wai.mapResponseHeaders (\hs -> if isServiceUnavailable response then h:hs else hs) response
isServiceUnavailable :: Wai.Response -> Bool
isServiceUnavailable response = Wai.responseStatus response == HTTP.status503

traceHeaderMiddleware :: AppConfig -> Wai.Middleware
traceHeaderMiddleware AppConfig{configServerTraceHeader} app req respond =
case configServerTraceHeader of
Nothing -> app req respond
Just hdr ->
let hdrVal = L.lookup hdr $ Wai.requestHeaders req in
app req (respond . Wai.mapResponseHeaders ([(hdr, fromMaybe mempty hdrVal)] ++))

I don't see an easy fix for those now so LGTM.

@steve-chavez steve-chavez merged commit 2825ac0 into PostgREST:main Sep 27, 2023
@develop7
Copy link
Collaborator Author

develop7 commented Sep 28, 2023

I don't see an easy fix for those now so LGTM.

um, why, the rest of module doesn't depend on them, so they could reside somewhere in/under App just as well

@develop7 develop7 deleted the feat-2771-response_pure branch September 28, 2023 09:33
@steve-chavez
Copy link
Member

um, why, the rest of module doesn't depend on them, so they could reside somewhere in/under App just as well

Ah right, simply moving them to another module would be fine.

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