-
Notifications
You must be signed in to change notification settings - Fork 118
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
Support for value classes #435
Conversation
# Conflicts: # src/test/tests.scala
# Conflicts: # src/test/SumsTests.scala
val res = SemiDefault.derived[WithDefault].default | ||
assertEquals(res, WithDefault(123)) | ||
} | ||
|
||
// test("serialize a value class") { |
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 are we missing to enable these tests?
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.
ah ok I see above :)
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.
maybe add a comment fo reach test why it fails?
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.
Added.
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.
Thanks - but what's a product value class? Do you mean case classes? If so, ServiceName1
is a case class, but it is disabled?
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.
Yes, I mean case classes - ServiceName
test is now enabled.
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.
there's some code duplication which I think we can remove
src/test/SumsTests.scala
Outdated
) | ||
assert( | ||
error contains "trait Parent is not a generic sum because its child trait BadChild is not a generic product because it is not a case class" | ||
error contains "Deriving the typeclass based on mirrors or directly is not possible for \"magnolia1.tests.SumsTests.Parent\":String. Please refer to the documentation or report a feature request." |
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's this :String
?
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.
Improved.
|
||
case t: TypeTree => t.tpe | ||
|
||
// case for AnyVal type annotations: with "-Yretain-trees" scalac option, the TypeTree of the annotation gets erased, |
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.
with the option, or without the option? It would seem on first sight that trees get erased without retain-trees
:)
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.
Tricky as it seems, it is with that option. It appears that -Yretain-trees
may change the types of the trees, f.e. scala/scala3#14195.
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.
Looks good, thanks! :)
Implements support for mirrorless value classes derivation for non-generic products with annotations and default values.
TODOs: