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

Multipart decoding using EntityDecoder #2821

Conversation

IndiscriminateCoding
Copy link
Contributor

@IndiscriminateCoding IndiscriminateCoding commented Aug 28, 2019

This PR is an attempt to improve multipart decoding by allowing to use EntityDecoder on a Part[F]:

  1. It introduces new interface Media[F] which is implemented by Message[F] and Part[F];
  2. EntityDecoder.decode now accepts a Media[F] instead of Message[F].

rossabaker
rossabaker previously approved these changes Aug 30, 2019
Copy link
Member

@rossabaker rossabaker left a comment

I think this is neat.

@ChristopherDavenport has long advocated for a typeclass approach to Message, and I'm guessing he won't like this. But the concept of Media might be a typeclass for which Request and Response and Part have instances.

I think the typeclass approach may be still compelling, but this makes a currently awkward thing less awkward, so I like it.

@rossabaker rossabaker dismissed their stale review Aug 30, 2019

Test failure is legit

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Aug 30, 2019

I'm so numb to the flaky tests that I didn't notice there's a tut failure here. We would need to clean that up first.

@IndiscriminateCoding
Copy link
Contributor Author

@IndiscriminateCoding IndiscriminateCoding commented Aug 30, 2019

Fixed

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Leaving a message that this concept digs further into a well I'm uncomfortable with. This is more unbound inheritance. Making it harder to go the other direction.

@IndiscriminateCoding
Copy link
Contributor Author

@IndiscriminateCoding IndiscriminateCoding commented Aug 31, 2019

@ChristopherDavenport another approach (without resorting to inheritance) would be to make Media a case class and store it in a fields of Request/Response/Part. But that would be more invasive change (and less efficient).

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 3, 2019

#1572 represents the typeclass approach. The diff is unfortunately difficult to read after I did something catastrophically stupid while working on docs, but the comments show the mostly pros and a few unresolved cons. I also think the typeclass approach might help with things like AuthedRequest, which I don't think was appreciated during the discussion of #1572.

I think the alternative to this PR is to have Media be a typeclass, like Message in #1572.

In either case, it would be nice if we had something that works for Part. The status quo is insufficient.

@IndiscriminateCoding
Copy link
Contributor Author

@IndiscriminateCoding IndiscriminateCoding commented Sep 3, 2019

Not sure if I like the (lawless) typeclass approach, tbh.
Basically you can just replace this typeclass with a bunch of lenses.
But (IMO) http4s API is already pretty complex (which is the price of MTL-like design).

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Sep 19, 2019

Would be nice to decide on this before 0.21 final, but are just looking at a milestone today.

@rossabaker rossabaker added this to the 0.21.0 milestone Nov 27, 2019
@hamnis
Copy link
Contributor

@hamnis hamnis commented Jan 7, 2020

I think this is reasonable and we should add this.

Copy link
Member

@rossabaker rossabaker left a comment

Between the two styles proposed, I come down on the side of working code. Someone can try the typeclass proposal after 0.21.

@ChristopherDavenport ChristopherDavenport merged commit fd66529 into http4s:master Jan 8, 2020
2 checks passed
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.

None yet

4 participants