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

Migrate JsonEncoder/Decoder macro to Scala 3 native macro. No more magnolia #929

Open
wants to merge 4 commits into
base: series/2.x
Choose a base branch
from
Open

Migrate JsonEncoder/Decoder macro to Scala 3 native macro. No more magnolia #929

wants to merge 4 commits into from

Conversation

987Nabil
Copy link
Contributor

  • annotation handling
  • decoders for recursive data structures

@987Nabil
Copy link
Contributor Author

Still needs some refactoring. But it would be nice, if one could approve the workflows.

987Nabil
987Nabil commented Apr 12, 2023
View reviewed changes
zio-json/shared/src/main/scala/zio/json/internal/readers.scala
@@ -161,7 +161,7 @@ final class WithRetractReader(in: java.io.Reader) extends RetractReader with Aut
private[zio] sealed trait RecordingReader extends RetractReader {
def rewind(): Unit
}
private[zio] object RecordingReader {
object RecordingReader {
def apply(in: OneCharReader): RecordingReader =
Copy link
Contributor Author

@987Nabil 987Nabil Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The macro basically inlines the code that calls this apply. Therefore we get a compile time error if it is package private.

@987Nabil 987Nabil changed the title Migrate JsonEncoder macro to Scala 3 native macro. No more magnolia Migrate JsonEncoder/Decoder macro to Scala 3 native macro. No more magnolia Apr 12, 2023
@987Nabil 987Nabil marked this pull request as ready for review April 14, 2023 06:32
@987Nabil 987Nabil requested a review from a team as a code owner April 14, 2023 06:32
@987Nabil
Copy link
Contributor Author

987Nabil commented Apr 14, 2023 •
edited
Loading

I thought one could extract the common macro code to some helper. But this does not seem to work since the imports of the using Quotes are path dependent. So I guess this is ready for review.

@987Nabil 987Nabil force-pushed the scala-3-native-macro branch from b387a71 to 8e40d52 Compare April 14, 2023 06:50
@jdegoes
Copy link
Member

jdegoes commented Apr 25, 2023

@987Nabil Looks great! Can you add some more tests for this under Scala 3 so we can be more assured the derivation is working correctly?

@987Nabil
Copy link
Contributor Author

There are actually a lot of shared tests. I think most cases are covered. But I can take a look if I find something missing.
Do you have something particular in mind?

@plokhotnyuk
Copy link
Contributor

plokhotnyuk commented Apr 25, 2023 •
edited
Loading

@987Nabil Using this benchmark set you can test both correctness and performance of the new way of derivation on ~60 synthetic and real-world JSON samples.

@987Nabil
Copy link
Contributor Author

fyi: The benchmark does not compile. Did not find the time yet to take a deeper look.

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.
3 participants
@987Nabil @jdegoes @plokhotnyuk