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

MessageFailure: simplify toHttpResponse/inHttpResponse #3110

Merged

Conversation

IndiscriminateCoding
Copy link
Contributor

@IndiscriminateCoding IndiscriminateCoding commented Jan 24, 2020

Applicative constraints on MessageFailure.inHttpResponse looks redundant because one can't do anything useful with them (given polymorphic context).

@IndiscriminateCoding
Copy link
Contributor Author

@IndiscriminateCoding IndiscriminateCoding commented Jan 24, 2020

Not sure why it fails on jdk8/scala-2.13

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 24, 2020

That failure is unrelated. It's one I'm hoping to fix with more consistent use of cats-effect-testing. I restarted the build.

@rossabaker rossabaker added this to the 0.21.0-RC2 milestone Jan 24, 2020
@rossabaker rossabaker added breaking breaking: source labels Jan 24, 2020
Copy link
Member

@rossabaker rossabaker left a comment

I think the old signature is a leftover from when encoding a body was effectful. This looks like a good simplification.

People can have custom MessageFailures, so this change could ripple outside our repo, but it's unlikely. I can't think of any legitimate uses it would preclude.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

This further solidifies the logging that happens as a side-effect. The F I think should wrap the logging and remain otherwise we are trapped in this non RT system. Just my thoughts.

@IndiscriminateCoding
Copy link
Contributor Author

@IndiscriminateCoding IndiscriminateCoding commented Jan 24, 2020

@ChristopherDavenport the issue is, it's impossible to do RT-logging using Applicative alone (it requires Sync at least).

@@ -134,13 +134,12 @@ package object server {
inDefaultServiceErrorHandler[F, F]

def inDefaultServiceErrorHandler[F[_], G[_]](
implicit F: Monad[F],
G: Applicative[G]): Request[G] => PartialFunction[Throwable, F[Response[G]]] = req => {
implicit F: Monad[F]): Request[G] => PartialFunction[Throwable, F[Response[G]]] = req => {
case mf: MessageFailure =>
messageFailureLogger.debug(mf)(
s"""Message failure handling request: ${req.method} ${req.pathInfo} from ${req.remoteAddr
Copy link
Member

@rossabaker rossabaker Jan 24, 2020

Choose a reason for hiding this comment

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

To @ChristopherDavenport's concern, the relevant logging I see is here, and in JsonDebugger. In both these places, we're returning in F and could compose an effect-tracked log with the response. But I don't see any logging where we're creating these responses, so the effect is more power than we want or need. Maybe I'm missing something, but I think both of you can be made happy?

Copy link
Member

@rossabaker rossabaker Jan 24, 2020

Choose a reason for hiding this comment

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

Excuse me. This one is missing Sync.

@rossabaker rossabaker merged commit 47af80a into http4s:master Jan 24, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants