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

A dash of API sugar to simplify summoning of implicit codec. #2030

Merged
merged 1 commit into from
Mar 9, 2017

Conversation

metasim
Copy link
Member

@metasim metasim commented Feb 27, 2017

Allows AvroRecordCodec[Tile] instead of implicitly[AvroRecordCodec[Tile]], a pattern used a lot in the avro2spark PoC.

Allows `AvroRecordCodec[Tile]` instead of `implicitly[AvroRecordCodec[Tile]]`, a pattern used a lot in the avro2spark PoC.
@pomadchin
Copy link
Member

pomadchin commented Feb 27, 2017

I like that idea, but mb to have a better name for it? like AvroRecordCodec.deriveCodec instead of apply. (yep, i know that your version looks a bit more pretty, but probably we can add semantics there?)

// circe styled example
import AvroRecordCodec._
val codec: AvroRecordCodec[Tile] = deriveCodec

@metasim
Copy link
Member Author

metasim commented Feb 27, 2017

I see it more like a type-level factory method. Like the apply on a companion object for a case class. I want a AvroRecordCodec[Tile], so I just ask the companion object for it, just like I would if I were dealing with a case class. So I think apply is appropriate. I can't remember where I first saw this technique, but was reminded of it here, described in this article

@lossyrob
Copy link
Member

In an interesting play on words, it feels like implicitly is more explicit than having an object apply do the work here: it hides a bit of the magic in an already magical function.

That being said, it does do the work of an object apply; it creates the thing that is the type of the companion object. If it's an established pattern that wouldn't throw people off, I'd say this change is cool. Should it be applied more broadly?

@echeipesh
Copy link
Contributor

It's consistent with object apply for sure. 👍 to making this code-base standard. Won't be able to apply to things outside our control like JsonFormat, but 🤷‍♂️ there.

@fosskers
Copy link
Contributor

fosskers commented Feb 27, 2017

Apologies, pressed the wrong button trying to cancel my comment.

In general, if this pattern means an increase in boilerplate and an increase in the number of entries for "machinery classes" that get rendered into Scaladocs, then I'm against it.

@metasim
Copy link
Member Author

metasim commented Mar 1, 2017

FWIW, it's also the pattern/convention used by avro4s. Just to be clear, I'm primarily interested in it just for this class; no stake in whether it's used more broadly.

@echeipesh echeipesh merged commit 5de5535 into locationtech:master Mar 9, 2017
@echeipesh echeipesh added this to the 1.1 milestone Mar 9, 2017
@metasim metasim deleted the avro-codec-sugar branch March 10, 2017 14:30
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.

5 participants