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

EntityEncoder as a typeclass with a pure operation #1694

Merged
merged 5 commits into from Mar 9, 2018

Conversation

Projects
None yet
4 participants
@jmcardon
Member

jmcardon commented Mar 8, 2018

This PR tackles eneity encoders as pure operations.

toEntity becomes Entity[F] instead of F[Entity[F]]

We could also tentatively consider EntityEncoder[A] with toEntity[F[_]: Sync]: Entity[F] given that you need at least Sync to run a stream. This universal quantification on the bound would also work, and it would remove the necessity of requiring more than one F[_] for an entitydecoder.

@jmcardon jmcardon changed the title from (WIP) EntityEncoder as a typeclass with a pure operation to EntityEncoder as a typeclass with a pure operation Mar 8, 2018

@rossabaker

I think this is a big win with some minor adjustments for compatibility.

case None => F.pure(w.headers)
}
hs.map(newHeaders => change(body = entity.body, headers = headers ++ newHeaders))
def withBody[T](b: T)(implicit w: EntityEncoder[F, T]): Self = {

This comment has been minimized.

@rossabaker

rossabaker Mar 9, 2018

Member

This breaks a lot of things. I wonder about deprecating this and introducing a new name for the pure one. How about withEntity?

This comment has been minimized.

@jmcardon

jmcardon Mar 9, 2018

Member

I wish I would've had that idea before I changed like 200 lines that depended on it :(

This comment has been minimized.

@jmcardon

jmcardon Mar 9, 2018

Member

jk IJ to the rescue :D

@@ -10,8 +10,9 @@ private[http4s] class MultipartEncoder[F[_]: Sync] extends EntityEncoder[F, Mult
//TODO: Refactor encoders to create headers dependent on value.
def headers: Headers = Headers.empty
def toEntity(mp: Multipart[F]): F[Entity[F]] =
Sync[F].delay(Entity(renderParts(mp.boundary)(mp.parts), None))
//Note: Is this actually effecting?

This comment has been minimized.

@rossabaker

rossabaker Mar 9, 2018

Member

I don't think so.

This comment has been minimized.

@jmcardon

jmcardon Mar 9, 2018

Member

Cool. That's what I thought, so I can alter the bound

You can seamlessly respond with a `Future` of any type that has an
`EntityEncoder`.
You can respond with a `Future` of any type that has an
`EntityEncoder` by lifting it into IO. Note: `Future` is a side effecting and

This comment has been minimized.

@rossabaker

rossabaker Mar 9, 2018

Member

Pedantry: future is pure. The problems are that it's strict (so creating a Future with an effect is an effect) and that mapping over it is an effect.

This comment has been minimized.

@SystemFw

SystemFw Mar 9, 2018

Contributor

s/effect/side effect.
You are correct, but how many people do you know that use Future with pure code inside? (I know of 0, they introduce Future to deal with side effects)

This comment has been minimized.

@rossabaker

rossabaker Mar 9, 2018

Member

I know of 0, but I know of non-zero who point this out every time someone calls Future impure on Twitter. :)

This comment has been minimized.

@SystemFw

SystemFw Mar 9, 2018

Contributor

But anyway you're right, this could probably we reworded like:

Note: unlike IO, wrapping a side effect in Future does not
suspend it, and the resulting expression would still be side 
effectful, unless we wrap it in IO.

or something like that

io.unsafeRunSync
```
As good functional programmers who like to delay our side effects, we
of course prefer to operate in [F]s:
```tut
val io = Ok(IO {
val io = IO {

This comment has been minimized.

@rossabaker

rossabaker Mar 9, 2018

Member

Would it be feasible to allow the old syntax with deprecation in http4s DSL? It would require something like:

@deprecated("flatMap that shit", "0.19")
def apply[F[_]](a: F[A])(implicit EE: EntityEncoder[F, A])

This overload is probably not going to fly with the existing one, but it's worth a shot if we can do compiler-assisted migration. This is a big breaking change.

Alternatively, it would be neat if we could target and scalafix the ones we ate.

jmcardon added some commits Mar 9, 2018

@rossabaker

I like it. And the migration path.

@ChristopherDavenport

👍 LGTM

@SystemFw

This comment has been minimized.

Contributor

SystemFw commented Mar 9, 2018

I don't want to dwell on this too long, but I think we should consider keeping the name withBody + a scalafix, instead of introducing withEntity + deprecation.

As a general position, I like deprecation better. But withEntity is a tiny bit obscure to the casual user, and withBody isn't. So which name do we want long term?

EDIT: actually a scalafix in this case would be hard... so the tradeoff might not be that great...

EDIT 2: ignore this, LGTM

@SystemFw SystemFw merged commit 3dda696 into http4s:master Mar 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rossabaker rossabaker referenced this pull request Apr 20, 2018

Open

Release strategy: 0.19 and 1.0 #1797

19 of 31 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment