-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
Wrap derived schema in Exported wrapper #594
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting! I have some suggestion to reduce breaking changes.
@@ -542,8 +546,7 @@ trait DerivationSchema[R] { | |||
private def getDescription[Typeclass[_], Type](ctx: ReadOnlyParam[Typeclass, Type]): Option[String] = | |||
getDescription(ctx.annotations) | |||
|
|||
implicit def gen[T]: Typeclass[T] = macro Magnolia.gen[T] | |||
|
|||
implicit def gen[T]: Exported[Typeclass[T]] = macro ExportedMagnolia.exportedMagnolia[Typeclass, T] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about calling this differently and make gen
return the same thing as before? So that we don't need to change every existing call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have some experiments with this.
- Old method
implicit def gen[T]: Typeclass[T] = Magnolia.gen[T]
Will pollute implicit scope when imported.
- Without implicit
def gen[T]: Typeclass[T] = Magnolia.gen[T]
Different semantic: magnolia disables recursive generation without implicit
keyword
https://github.com/propensive/magnolia/blob/master/core/shared/src/main/scala/magnolia.scala#L249-L251
- Using implicit
def gen[T](implicit e: Exported[Typeclass[T]]) = e.instance
Works with default schema (Schema.gen[MyClass]
) but not work with custom schemas like this:
object mySchema extends GenericSchema[Blocking]
mySchema.gen[MyClass] // this will fail
I will experiment with this a bit more on weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you renamed the implicit gen
into e.g. autogen
and added a new def gen
that is not implicit and just a shortcut to avoid calling .instance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 3rd one is acceptable, actually the caliban docs say you should import mySchema._
when you use this pattern, which makes the implicit in scope. I opened #658 to get this in.
Closing since #658 was merged |
This PR fights with issue described on this issues:
Short version: macro derivation sometimes conflicts with builtin schemas defined in Schema companion object.
On this PR I followed solution described by @neko-kai on this comment.
In first commit I added failing test so it can be tested it in isolation.