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.

Sign up for GitHub

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

Jump to bottom

scala2 DeriveSchema intermediate enum fix #749

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

scala2 DeriveSchema intermediate enum fix #749

wants to merge 8 commits into from

Conversation

googley42
Copy link
Contributor

@googley42 googley42 commented Oct 12, 2024 •
edited
Loading

fixes #747

There is inconsistent behaviour for EnumN derivation between Scala 2 and 3 when dealing with nested sealed trait hierarchies

googley42
googley42 commented Oct 12, 2024
View reviewed changes
zio-schema-avro/src/test/scala/zio/schema/codec/AvroSchemaCodecSpec.scala
@@ -81,7 +81,7 @@ object AvroSchemaCodecSpec extends ZIOSpecDefault {
val expected =
"""[{"type":"record","name":"A","fields":[]},{"type":"record","name":"B","fields":[]},{"type":"record","name":"MyC","fields":[]},{"type":"record","name":"D","fields":[{"name":"s","type":"string"}]}]"""
assert(result)(isRight(equalTo(expected)))
} @@ TestAspect.scala2Only,
} @@ TestAspect.scala2Only @@ TestAspect.ignore,
Copy link
Contributor Author

@googley42 googley42 Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this test was created as a workaround for the incorrect Scala2 DeriveSchema behaviour for intermediate traits - it now fails due to the fix as expected - so its OK to ignore/delete this test? cc @devsprint

@googley42 googley42 marked this pull request as ready for review October 12, 2024 12:39
@googley42 googley42 requested a review from a team as a code owner October 12, 2024 12:39
googley42
googley42 commented Oct 12, 2024
View reviewed changes
zio-schema-derivation/shared/src/main/scala-2/zio/schema/DeriveSchema.scala
@@ -559,7 +559,7 @@ object DeriveSchema {
child.typeSignature
val childClass = child.asClass
if (childClass.isSealed && childClass.isTrait)
knownSubclassesOf(childClass)
Set(childClass.asType.toType)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop recursion if child is sealed trait

@googley42
Copy link
Contributor Author

googley42 commented Oct 12, 2024 •
edited
Loading

actually, gonna try and write a custom unit test for this

@googley42
Copy link
Contributor Author

googley42 commented Oct 13, 2024 •
edited
Loading

@987Nabil / @jdegoes ready for review pls when you have some time

@googley42 googley42 changed the title scala2 Derive intermediate enum fix scala2 DeriveSchema intermediate enum fix Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DeriveSchema macro - inconsistent behaviour for Enum N derivation between Scala 2 and 3
1 participant
@googley42