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

Implement support for > 22 field case classes in macros #3

Open
gmethvin opened this Issue Dec 12, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@gmethvin
Copy link
Member

gmethvin commented Dec 12, 2016

(Moved from playframework/playframework#3174)

In Scala 2.11, a case class can have more than 22 fields. We could modify our existing macros to support these. Here's approximately how to do it:

  • Detect > 22 field case class - if it's not greater than 22 fields, then simply generate reads/writes identical to the way they are generated now.
  • Group fields into groups of 22 (or less).
  • Generate reads/writes for tuples of each of the groups fields
  • Generate reads/writes for the case class that combines the reads/writes for the groups. It will have to use the constructor to create the class, and access the fields directly to write the class into a set of tuples, rather than using the apply/unapply methods of the companion object.

Then we should be able to support case classes with 484 parameters. I don't know if it's possible to create such a thing, since I think there's a 255 parameter limit for methods.

@cvogt

This comment has been minimized.

Copy link

cvogt commented Dec 12, 2016

for reference see earlier discussion on playframework/playframework#3174 (comment)

@kevinmeredith

This comment has been minimized.

Copy link

kevinmeredith commented Feb 1, 2017

Assuming that each bullet point, @gmethvin, represents the code/logic of this macro implementation, can you please explain why the following step is necessary?

Group fields into groups of 22 (or less).

@gmethvin

This comment has been minimized.

Copy link
Member

gmethvin commented Feb 2, 2017

Actually @jroper wrote the description originally (I just copied it from playframework/playframework#3174). He was suggesting we use tuples so we can use the existing combinators we have, and just combine everything at the end. It's explained in more detail on the original playframework issue.

@wsargent

This comment has been minimized.

Copy link
Member

wsargent commented Jun 15, 2017

@magro

This comment has been minimized.

Copy link

magro commented Oct 28, 2017

Are there plans to work on this, maybe even a rough ETA?

@cvogt

This comment has been minimized.

Copy link

cvogt commented Oct 28, 2017

If somebody wants to merge play-json-extensions into play-json, I'd be more than happy.

@gmethvin

This comment has been minimized.

Copy link
Member

gmethvin commented Oct 28, 2017

We actually already support most of the features there (tuples, default values, sealed traits). I suspect we probably just want to rewrite the Json macro to use the logic used in play-json-extensions.

We don't have any current plans to work on this but we're happy to accept pull requests.

I also think this could improve performance. The way we're doing it now uses the functional syntax to "build" the serializer, which creates a lot of extra function calls and objects. The more direct style used in play-json-extensions would likely perform better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment