Add support for 22+ case class in format macros - #1368
Open
Iilun wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 !