Extension of JsonCodec to have parity features with generic.extra #91
Conversation
modules/derivation/shared/src/main/scala/io/circe/derivation/DerivationMacros.scala
Outdated
Show resolved
Hide resolved
|
|
||
| if (noSerializeDefault && defaultValue.isDefined) { | ||
| q""" | ||
| if(a.$name==${defaultValue.get}) None else |
There was a problem hiding this comment.
Do you mind putting the prefixes on None and Some for the sake of hygiene?
There was a problem hiding this comment.
Could you better explain the change that you want? Witch prefix?
There was a problem hiding this comment.
The problem is that if the user has a different None definition at the macro use site, it'll get used instead of scala.None.
modules/derivation/shared/src/main/scala/io/circe/derivation/DerivationMacros.scala
Outdated
Show resolved
Hide resolved
| sealed trait Discriminator | ||
|
|
||
| object Discriminator { | ||
| final case class Embedded(fieldName: String) extends Discriminator |
There was a problem hiding this comment.
In circe-generic-extras we tend to use "type field" and "object wrapper" to refer to these approaches. I don't feel too strongly about this, but I think we should standardize one way or the other.
There was a problem hiding this comment.
Let's me check and apply the same approach
| final case class Embedded(fieldName: String) extends Discriminator | ||
| final case object TypeDiscriminator extends Discriminator | ||
|
|
||
| lazy val default = Embedded("type") |
There was a problem hiding this comment.
Nitpicking again, but can you put a type annotation on this, both for documentation and so it's not inferred to be Embedded?
There was a problem hiding this comment.
Also for something like this I think the overhead of lazy val probably outweighs any benefits of deferring the instantiation.
There was a problem hiding this comment.
I used the lazy val to be sure to no have concurrency problem in initialization of the value
| val tpe = weakTypeOf[T] | ||
|
|
||
| // Valid only in macro!! | ||
| val globalUseDefaults: Boolean = extractUseDefaults(useDefaults.tree) |
There was a problem hiding this comment.
Not sure about the name here. In what sense is it global?
There was a problem hiding this comment.
I'll rename in useDefaults
modules/derivation/shared/src/main/scala/io/circe/derivation/package.scala
Show resolved
Hide resolved
|
I think the failure is just Scalafmt— |
|
|
||
| /** Configuration allowing customisation of JSON produced when encoding or | ||
| * decoding. | ||
| * | ||
| * This configuration creates *both* encoder and decoder. | ||
| */ | ||
| final case class Codec( | ||
| transformMemberNames: String => String | ||
| transformMemberNames: String => String, |
There was a problem hiding this comment.
Now that we support ADTs we should probably have transformConstructorNames as well, but I can add that in a follow-up.
|
We will want to do some benchmarking to ensure that this doesn't impose a compile-time cost on the use cases we support now, but we can worry about that later. |
|
Oh, you also need |
| @@ -87,11 +87,11 @@ private[derivation] final class GenericJsonCodecMacros(val c: blackbox.Context) | |||
|
|
|||
| private[this] def codecFrom(tree: Tree): JsonCodecType = | |||
| tree.tpe.dealias match { | |||
| case t if t <:< typeOf[Configuration.Codec] => | |||
| case t if t == typeOf[Configuration.Codec] => | |||
There was a problem hiding this comment.
equals is not guaranteed to work correctly on scala-reflect types. Maybe =:=?
There was a problem hiding this comment.
Good catch, I hadn't noticed these changes. @aparo, is there a reason to change these? I fixed this a few weeks ago because I was getting inaccurate warnings in some contexts.
|
Merging this, and will address remaining issues in a follow-up. Thanks again so much, @aparo! |
|
Oh, there's a merge conflict. I'll pull these commits into a new PR. |
|
Included in #100. |
The implementation allows to:
I closes #8 and #24