-
-
Notifications
You must be signed in to change notification settings - Fork 134
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
Fix #728 - Support case class with more 22 fields in Scala 3 #729
Conversation
2d5dedf
to
989bddf
Compare
} | ||
|
||
"from generic Bar" in { | ||
testWithTuple( | ||
Bar[Double]("bar1", None, Seq(1.2D, 34.5D)) | ||
).mustEqual( | ||
"scala.Tuple3[scala.Predef.String, scala.Option[scala.Double], scala.collection.immutable.Seq[scala.Double]]/Bar(bar1,None,List(1.2, 34.5))" | ||
"play.api.libs.json.Bar[scala.Double]/Bar(bar1,None,List(1.2, 34.5))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case class type is directly usable (and more specific)
val fieldNme = s"_${i + 1}" | ||
|
||
val resolve: Term => Term = { | ||
val field = tupleTpeSym.declaredField(fieldNme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either resolve as field or nullary method
tt: Type[T], | ||
ut: Type[U] | ||
): Tuple2[TypeRepr, Expr[T] => (Expr[U] => Expr[R]) => Expr[R]] = | ||
TypeRepr.of(using ut) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directly use the given product type
"no custom ProductOf" in { | ||
"Macros.writer[CustomNoProductOf]".mustNot(typeCheck) | ||
"be handled" when { | ||
"is declared with more than 22 fields" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failing before
989bddf
to
e0ae719
Compare
e0ae719
to
4880314
Compare
@@ -227,7 +227,8 @@ object Json extends JsonFacade with JsMacros with JsValueMacros { | |||
JsValueWrapperImpl(w.writes(field)) | |||
|
|||
def obj(fields: (String, JsValueWrapper)*): JsObject = JsObject(fields.map(f => (f._1, unwrap(f._2)))) | |||
def arr(items: JsValueWrapper*): JsArray = JsArray(items.iterator.map(unwrap).toArray[JsValue]) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor formatting
@@ -143,6 +161,66 @@ case class Foo(bar: String, lorem: Int) | |||
|
|||
case class Bar[T](name: String, opt: Option[T], scores: Seq[Double]) | |||
|
|||
case class BigFat( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case class with 26 fields
@ihostage is there a reason why this PR doesn't have any comments/feedback? I've run it locally and it seems to be working fine. The tests pass so I wonder why it's stopping us from merging and do a cutover for RC7. |
No special reason. Probably just got out of the radar. There is something wrong with CI, so I will close it and reopen to retrigger it, but I think it's ready for merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix #728