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

Add a type class combining encoding and decoding #133

Closed
travisbrown opened this issue Dec 1, 2015 · 3 comments
Closed

Add a type class combining encoding and decoding #133

travisbrown opened this issue Dec 1, 2015 · 3 comments

Comments

@travisbrown
Copy link
Member

Today on Gitter @julienrf asked the following question:

@travisbrown can you remind me why you decided to remove CodecJson?

And my response:

@julienrf Primarily because I think the role it plays in Argonaut is confusing. It can be handy for definitions, but you generally don't want to use it as a context bound anywhere except tests, since there aren't CodecJson instances for lots of things that have DecodeJson and EncodeJson instances…
@julienrf …and that's the case because you can't define e.g. CodecJson[String] in the CodecJson companion object, since then it wouldn't be found when you ask for DecodeJson[String].
@julienrf So you could either put all your CodecJson instances for these basic types in some object that you expect users to import (ugh), or have some kind of implicit that automatically combines DecodeJson and EncodeJson into a CodecJson (which was removed in Argonaut for reasons I don't exactly remember, although I can imagine how that might get messy), or do what Argonaut does and just provide CodecJson for convenient definitions and let users figure out why it's pretty much useless as a requirement.
None of those options are very nice, so at the beginning I decided to leave it out of circe entirely.
It's possible that it's possible to do it right (or at least dramatically better) with export-hook, and I'd definitely be open to that possibility.

My wishlist for a Codec type class in circe would look something like this:

  1. A Codec[A] should be available (without any imports) for any A that has a Decoder and Encoder—i.e. if io.circe.Decoder[A] and io.circe.Encoder[A] compile, then io.circe.Codec[A] must compile as well.
  2. All tests should pass as currently written.
  3. If a type has a Codec instance, then asking for a Decoder (or Encoder) for that type shouldn't require additional allocations.
  4. We shouldn't have to change the name of the apply methods on Decoder or Encoder.
  5. It should be possible to define Codec instances for standard library and circe types in the Codec companion object and have Encoder and Decoder instances available with no imports.

Only the first two are hard requirements, and the third and fourth are probably incompatible.

@tpolecat
Copy link

tpolecat commented Dec 1, 2015

FWIW I just went through this in reverse with doobie (and had a good discussion with @mpilquist who went through a similar exercise with scodec). My approach is:

  1. All code is written in terms of Decode and Encode; if you need both you ask for both.
  2. Codec is just a convenience for the common case where you have a symmetric pair and want to define them together (or xmap an existing pair). It's not used as a context bound.
  3. Encode and Decode have implicit derivations that consume and return a Codec to fix the search problem described above.
  4. You can't get a Codec implicitly from an Encode + Decode. I think this turns out to not be very useful because of (1).

In my case you almost always have read/write pairs so getting rid of Codec would be really inconvenient.

@tpolecat
Copy link

tpolecat commented Dec 1, 2015

Also if you add Codec[A] you might consider whether the more general profunctor GenCodec[A,B] (name stolen from scodec) is interesting enough to keep around. It kind of pops out. I have it in doobie for now but I'm kind of on the fence.

@travisbrown
Copy link
Member Author

Done in #1151.

aeons pushed a commit to http4s/http4s that referenced this issue Aug 11, 2019
Rob Norris's third bullet point, circe/circe#133 (comment), motivated this change.
julienrf pushed a commit to scalacenter/circe that referenced this issue May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants