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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix EntityResponseGenerator #2785

Merged
merged 1 commit into from Aug 14, 2019

Conversation

@rintcius
Copy link
Contributor

@rintcius rintcius commented Aug 13, 2019

We got a failing test in our test suite after trying to upgrade http4s. After some digging it appeared to be introduced in #2446

This PR fixes the failing test for us.

Take body: F[A] as parameter instead of G[A] which enables following the code to be structured as it was here https://github.com/http4s/http4s/pull/2446/files#diff-09e729dc8ff0315c384e774351a8720fL42

If 馃憤 I'm happy to make analogous change to other branches.

Copy link
Member

@rossabaker rossabaker left a comment

The failing tests are related to #2782. I restarted them.

I'm surprised that this is binary compatible, but type erasure saves us here. This is source incompatible, but I think better reflects the intent.

No need to port to other branches: we'll merge series/0.20 to master.

Loading

@rossabaker rossabaker merged commit ae8a0c2 into http4s:series/0.20 Aug 14, 2019
1 of 2 checks passed
Loading
@djspiewak
Copy link
Contributor

@djspiewak djspiewak commented Aug 14, 2019

I'm surprised that this is binary compatible, but type erasure saves us here

Unless there are some constraints on G, I'm pretty sure this is binary incompatible just in a way that mima won't catch.

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 14, 2019

The constraint is EntityEncoder[G, A].

For most people, F is G. And this will be mostly applications calling it. But, eh, maybe this would cause a ClassCastException in the encoder for the unlucky few? 馃

Loading

@djspiewak
Copy link
Contributor

@djspiewak djspiewak commented Aug 14, 2019

Could also cause a ClassCastException in the Monad[F].flatMap. I do agree that for most people, those two would probably be unified. I guess it depends on how fussed you want to be about it. In theory, the old function was just鈥β燽roken鈥 so even breaking that code is kind of justified? I dunno.

Loading

@hamnis
Copy link
Contributor

@hamnis hamnis commented Aug 16, 2019

This broke the DirectivesDsl.
I suppose we will have to live with:

Ok(Directive.liftF(IO("Output")))

Loading

@hamnis
Copy link
Contributor

@hamnis hamnis commented Aug 16, 2019

If we instead change this to

  def apply[A](body: G[A])(implicit nat: G ~> F ,F: Monad[F], w: EntityEncoder[G, A]): F[Response[G]] =
    F.flatMap(nat[A](body))(apply[A](_))

then it works in both cases

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 16, 2019

Sorry. I thought it was a mistake, but I remember that Http4sDsl2 was added for the directives DSL, and should have tried that or pinged you.

Since it broke working code, and is of questionable binary compatibility, I think we should revert for 0.20.

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 16, 2019

As for 0.21, I don't think of ~> as something we usually pass around as implicit evidence. I wonder about just giving the F version a different name than apply.

Loading

@hamnis
Copy link
Contributor

@hamnis hamnis commented Aug 16, 2019

We might then need a different way of plugging in the lifting of G to F in 0.21 ? I can try to come up with something.

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 16, 2019

Yeah, my first priority here is that we not break http4s-directives. If we can come up with something clean for both that and the use case that motivated this original PR, that's the best thing for 0.21... and if it's compatible, for 0.20.

Loading

@rintcius rintcius deleted the fix-EntityResponseGenerator branch Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants