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

Implement optional implicit allowing usage of circe decoders without EntityDecoder definition #1917

Merged
merged 5 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@Leammas
Contributor

Leammas commented Jun 7, 2018

I've discussed the necessity of this implicit in gitter. Appeared useful.

@rossabaker

I think this is useful, but it's worth mentioning two reasons that this is not a default, and maybe even worth mentioning this in the docs.

  1. Many APIs support multiple representations. There's one reasonable way to render a Json, but there can be multiple reasonable ways to render a Foo.

  2. This introduces incoherent instances. For instance, now we have two EntityDecoder[F, String]. It surprises people which one wins.

If that doesn't scare people off, this technique is often useful.

I don't see any reason not to do an Encoder where we do the Decoder. And in that case, it'd be nice to not require two imports.

@Leammas

This comment has been minimized.

Contributor

Leammas commented Jun 8, 2018

The first point is already mentioned in https://github.com/http4s/http4s/pull/1917/files#diff-dcf0079785763cffc8cb901dad9f0628R205 Should I clarify it?

And I doubt that incoherent instances are possible in the same scope: they result in ambiguous implicits error.

@rossabaker

This comment has been minimized.

Member

rossabaker commented Jun 10, 2018

I looked at the diff, and not the wider context. You're right, I think the first point is covered well enough.

As for the second issue, a lexically scoped instance (e.g., import CirceEnityDecoder._) has higher priority than an implicitly scoped instance (e.g., one found on the companion of EntityDecoder). So the issue here is that Ok("foo") compiles with or without this new import, but the behavior varies. It's fine as long as people know the ramifications, but it's a situation that I personally prefer to avoid.

@Leammas

This comment has been minimized.

Contributor

Leammas commented Jun 14, 2018

I tried testing the implicit resolution and found that it does not compile at all. Somehow EntityDecoder from companion and generic one got the same priority.
Here's the repro: https://github.com/Leammas/http4s/blob/4f114c4ecccc3ab01a6b0491968056de527cdd5c/circe/src/test/scala/org/http4s/circe/CirceSpec.scala#L191

/**
* Derive [[EntityDecoder]] if implicit [[Decoder]] is in the scope without need to explicitly call `jsonOf`
*/
trait CirceEntityDecoder {

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Jun 14, 2018

Member

Why do this as a trait and then an object which extends said trait? Why not just place this in the object?

This comment has been minimized.

@Leammas

Leammas Jun 14, 2018

Contributor

I use it in combined object org.http4s.circe.CirceEntityEncDec now and there's a small probability that someone may find it useful for its own generic code.

@Leammas

This comment has been minimized.

Contributor

Leammas commented Jun 14, 2018

I added implementation allowing usage of EntityEncoder in the same way and the single import mentioned by @rossabaker

@@ -0,0 +1,3 @@
package org.http4s.circe
object CirceEntityEncDec extends CirceEntityDecoder with CirceEntityEncoder

This comment has been minimized.

@rossabaker

rossabaker Jun 15, 2018

Member

I think Codec is a more common term than EncDec. How about CirceEntityCodec?

This comment has been minimized.

@Leammas

Leammas Jun 15, 2018

Contributor

Yeah, sounds better.

and decoding derivation:
```tut:book
org.http4s.circe.CirceEntityEncDec._

This comment has been minimized.

@rossabaker

rossabaker Jun 15, 2018

Member

This tut broke with an ambiguous implicit. We need to sort this out. Running sbt doc/tutQuick should test it locally.

This comment has been minimized.

@Leammas

Leammas Jun 15, 2018

Contributor

Woops, the exact thing I wrote: we can't use both CirceEntityEncoder and plain json. I restricted the scope, but I'm not sure if it's the right way. Maybe I should not use tut:book there at all?

This comment has been minimized.

@rossabaker

rossabaker Jun 16, 2018

Member

I think it's good to ensure that the import continues to compile, and you note the risk of ambiguity below. I played with invisible braces around it, and it doesn't work. I think the best we can do.

@rossabaker

I can backport this into release-0.18 if we want to squeeze this into the release with the pool fix.

@rossabaker rossabaker merged commit a53dbb1 into http4s:master Jun 26, 2018

2 checks passed

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

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 26, 2018

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 26, 2018

@Leammas Leammas deleted the Leammas:add_implicit_decoder branch Jun 26, 2018

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 28, 2018

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 29, 2018

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 29, 2018

rossabaker added a commit to rossabaker/http4s that referenced this pull request Jun 29, 2018

rossabaker added a commit that referenced this pull request Jun 29, 2018

Merge pull request #1930 from rossabaker/backport-1917
Backport #1917 (CirceEntityCodec) to 0.18.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment