Add support for 22+ case class in format macros - #1368
Conversation
| private def pads(n: Int): JsObject = JsObject(1.to(n).map(i => s"pad$i" -> JsNumber(i))) | ||
|
|
||
| "Reads for classes with more than 22 fields" should { | ||
| "be generated for simple case class" in { |
There was a problem hiding this comment.
Up-to-22 case already covered, useful to add this one?
There was a problem hiding this comment.
Since the code path between 1 to 22 fields and > 22 fields differs, I did find it necessary to ensure it behaved the same, replicating all tests from the existing suite to ensure > 22 case classes on the new code path behave the same as < 22 case classes on the previous code path.
During my development, some wiring mistakes did cause some of these tests to fail, which makes me think that they are indeed necessary to avoid regressions.
However, this is a direct consequence of having two very distinct code paths, but as I wrote in the description, it seemed the best way for me.
| ) { _.reads(json).mustEqual(JsSuccess(expected)) } | ||
| } | ||
|
|
||
| "as Format for a simple generic case class" in { |
There was a problem hiding this comment.
It is possible to make this one fail by editing L762 of JsMacroImpl, removing the asSeenFrom, without making the < 22 test case fail.
As such, I would say that it is required, but I can edit it if necessary
There was a problem hiding this comment.
Format for case class < 21 were already covered for me, so adding other test about doesn't improve the coverage (but as a maintenance cost).
There was a problem hiding this comment.
I have removed every test that could not be made to fail only on the > 22 branch, or that was redundant.
Should be good but please let me know if I still missed some !
There was a problem hiding this comment.
Most of the added tests I still see there are still <= 22, so redundant.
There was a problem hiding this comment.
Looking at the existing tests layout, tests should rather be added in MacroScala2Spec (22+ already covered for Scala 3 in MacroScala3Spec.scala)
There was a problem hiding this comment.
I had missed that.
I will move all my tests to MacroScala2Spec. If you wish, I can also reduce the tests to a similar wording and coverage of MacroScala3Spec, but I do think this could introduce regressions later on (since it is not one unified code path for the whole scala 2 macro).
There was a problem hiding this comment.
- Tests for case 22- are already covered, duplicate tests must not be introduced.
- Specific changes must be tested (Scala 2 case 22+)
| } | ||
| } | ||
|
|
||
| "ignore Option alias" in { |
There was a problem hiding this comment.
This one is indeed in shared code and as such is redundant.
It comes from an overabundance of caution on my part, really wanting to have an exact behavior replica and avoid wiring mistakes. Do you want me to remove it ?
1375d9f to
3c9d8e7
Compare
2 participants
Pull Request Checklist
Fixes
Fixes #3
Purpose
This PR introduces support for macros for case classes with more than 22 fields.
Background Context
The existing macro could not handle case classes with more than 22 fields and failed with an error
No unapply or unapplySeq function found for ....Implementation choices
My goal in this implementation was to keep all existing code paths exactly the same, and for the new approach, take inspiration from the play-json-extensions macro.
This was to ensure that if someone used custom apply / unapply, it would not be impacted, and my new implementation only starts when the aforementioned error would otherwise have surfaced, only introducing a new behavior, and not affecting existing code.
This does mean that I had to duplicate the test coverage, since the code paths for case class <22 and 22+ are different. This produces a quite heavy diff, I am unsure what strategy to adopt here.
I also avoided messing too much with the utility CaseClass class, as its code is not easily readable, and only edited it to reuse existing code.
If this choice does not meet the repo criteria, I will happily refactor this, to have a uniform code path if required (however I am afraid of regressions with such an approach, since the previous macro approach had a lot of quirks)
Docs
I searched the documentation, but it mentioned that case class were supported, so I wasn't sure what to add to it, left it as is for now.
Thank you for your feedback !