Skip to content

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

Merged
echeipesh merged 1 commit into
locationtech:masterfrom
metasim:avro-codec-sugar
Mar 9, 2017
Merged

echeipesh merged 1 commit into
locationtech:masterfrom
metasim:avro-codec-sugar

Conversation

@metasim

@metasim metasim commented Feb 27, 2017

Copy link
Copy Markdown
Member

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

pomadchin commented Feb 27, 2017 •

Copy link
Copy Markdown
Member

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

metasim commented Feb 27, 2017

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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

fosskers commented Feb 27, 2017 •

Copy link
Copy Markdown
Contributor

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

metasim commented Mar 1, 2017

Copy link
Copy Markdown
Member Author

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