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.

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

Feat: scala3 enumeration support #1068

Merged
merged 11 commits into from
Jun 14, 2024

Conversation

ThijsBroersen
Copy link
Contributor

@ThijsBroersen ThijsBroersen commented Feb 21, 2024

With this PR I have tried to implement Scala 3 enumeration support. Because the current macros already do some assumptions and are build with Magnolia I did not want to disrupt to much and tried to implement it under the following condition:
If all subtypes are named values (objects without parameters) then the type is an enumeration.

This means that if an enum has mixed children, with and without parameters, then the implementations stays the same. Only for pure enumerations (all named values) the encoding/decoding will be affected.

Breaking:
To my opinion the current implementation is wrong as enumerations are normally serialized as strings. But this PR will of course be breaking for the few using zio-json with Scala 3 enumerations already.

I deliberately did not try to implement this for Scala 2 as these are not native enumerations.

@ThijsBroersen ThijsBroersen requested a review from a team as a code owner February 21, 2024 12:48
@@ -490,16 +515,18 @@ object DeriveJsonDecoder extends Derivation[JsonDecoder] { self =>
}
}

private lazy val caseObjectEncoder = new JsonEncoder[Any] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This static encoder make it possible to filter on at L625

@@ -595,13 +622,43 @@ object DeriveJsonEncoder extends Derivation[JsonEncoder] { self =>
}

def split[A](ctx: SealedTrait[JsonEncoder, A]): JsonEncoder[A] = {
val isEnumeration = ctx.subtypes.forall(_.typeclass == caseObjectEncoder)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to use this hack because I don't think I have access to the ctx.params of the subtypes here anymore.

@ThijsBroersen
Copy link
Contributor Author

Interesting, so it seems shared specs are now failing because in Scala 3 the sealed traits are now also treated as enumerations. 🤔

@ThijsBroersen
Copy link
Contributor Author

Interesting, so it seems shared specs are now failing because in Scala 3 the sealed traits are now also treated as enumerations. 🤔

Fixed 🙃

@fsvehla
Copy link
Contributor

fsvehla commented Mar 18, 2024

It’d say let’s get it in, the next version will be a new 0.x.
Do you have a few cycles to add some docs?

@ThijsBroersen ThijsBroersen force-pushed the feat-scala3-enumeration-support branch from 1187ebf to eeb0df0 Compare March 30, 2024 19:16
@ThijsBroersen
Copy link
Contributor Author

@fsvehla I was just trying to update the docs but this docs modules is Scala 2.13 only. Can Scala 3 be mixed in?

@ThijsBroersen
Copy link
Contributor Author

@fsvehla I did one more attempt:

  • added docs with non-mdoc Scala 3 code-blocks
  • adjusted the implementation to also treat sealed family (where all members are objects) as enums

The tests now fail with the GoldenSpec which assumes sealed families with only object members should be encoded as objects...
I don't agree with the spec as many libraries, including Tapir, encode this as string-based enum. When using Tapir this would lead to an endpoint schema which renders an openapi spec saying the type is a string-based enum and a json codec which behaves differently.

What is your view on this?

@fsvehla
Copy link
Contributor

fsvehla commented Jun 13, 2024

@ThijsBroersen I agree that the proposed change makes sense for most cases (and takes care to exclude enums containing case classes). Fine to remove that test!

@ThijsBroersen
Copy link
Contributor Author

Changed the golden-test SumType.Case3 from case object Case3 to case class Case3(). Now it is derived as non-enum and the test works as expected. So this PR will change sealed traits / enums where all cases are without constructors, these are now derived as string-based enums.

@ThijsBroersen ThijsBroersen force-pushed the feat-scala3-enumeration-support branch from 12ee766 to 7425dd3 Compare June 13, 2024 18:20
@ThijsBroersen
Copy link
Contributor Author

Lets merge!

@ThijsBroersen ThijsBroersen force-pushed the feat-scala3-enumeration-support branch from 7425dd3 to 7d26898 Compare June 14, 2024 05:57
@fsvehla fsvehla merged commit 5773f6c into zio:series/2.x Jun 14, 2024
28 checks passed
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.

None yet

2 participants