-
-
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
Cross-build to Scala 3 #557
Conversation
0c745b5
to
dbd8dd7
Compare
645d087
to
5821997
Compare
e98f0e5
to
d2d9a46
Compare
Ready for review @marcospereira @ennru @raboof @cchantep @jtjeferreira @ignasi35 @octonato. I'd like to release this sooner rather than later to give users (e.g. #539 @rossabaker in http4s) a chance to test Scala 3 before 3.0 is released (ideally before its RC1 is released). |
Let me know if you prefer a video call so I can run you through the PR, answer any concerns, clarify things and take feedback. |
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. I didn't finish looking into all files, but left some comments
docs/manual/working/scalaGuide/main/json/code/ScalaJsonAutomatedSpec.scala
Outdated
Show resolved
Hide resolved
play-json/shared/src/test/scala-3/play/api/libs/json/ScalaTestPosition.scala
Show resolved
Hide resolved
play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala
Outdated
Show resolved
Hide resolved
play-json/shared/src/main/scala-3/play/api/libs/json/JsMacroImpl.scala
Outdated
Show resolved
Hide resolved
play-json/shared/src/main/scala/play/api/libs/json/JsLookup.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: João Ferreira <jtjeferreira@gmail.com>
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.
Lot of work 👍
Have some questions.
docs/manual/working/scalaGuide/main/json/code/ScalaJsonAutomatedSpec.scala
Show resolved
Hide resolved
play-json/shared/src/test/scala/play/api/libs/json/WritesSharedSpec.scala
Show resolved
Hide resolved
Co-authored-by: Cédric Chantepie <cchantep@users.noreply.github.com>
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.
Thanks for the thorough review, @cchantep! Sorry I missed the notification yesterday.
docs/manual/working/scalaGuide/main/json/code-2/Scala2JsonAutomatedSpec.scala
Outdated
Show resolved
Hide resolved
docs/manual/working/scalaGuide/main/json/code/ScalaJsonAutomatedSpec.scala
Show resolved
Hide resolved
// Each JSON objects is marked with the admTpe, ... | ||
discriminator = "admTpe", | ||
// ... indicating the lower-cased name of sub-type | ||
typeNaming = JsonNaming { fullName => |
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.
Needed change because of a limitation in Dotty's typeclass derivation: scala/scala3#11048. You can also see a reference to that ticket in this diff.
play-functional/src/main/scala/play/api/libs/functional/Applicative.scala
Show resolved
Hide resolved
play-json/shared/src/test/scala/play/api/libs/json/WritesSharedSpec.scala
Show resolved
Hide resolved
docs/manual/working/scalaGuide/main/json/code-2/Scala2JsonAutomatedSpec.scala
Outdated
Show resolved
Hide resolved
// Each JSON objects is marked with the admTpe, ... | ||
discriminator = "admTpe", | ||
// ... indicating the lower-cased name of sub-type | ||
typeNaming = JsonNaming { fullName => |
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.
Meaning there is a regression for the existing Scala 2 play-json?
@@ -55,7 +55,7 @@ class JsonSpec extends org.specs2.mutable.Specification { | |||
) | |||
) | |||
.inmap(optopt => optopt.flatten, (opt: Option[Date]) => Some(opt)) | |||
)(Post, unlift(Post.unapply)) | |||
)(Post.apply, p => (p.body, p.created_at)) |
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.
Ok but it should be kept until next major release for the Scala 2 version?
Please let me know if there's any remaining feedback that needs to be addressed, otherwise I would like to merge and release this week. |
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 for releasing an RC to unblock upstream users.
However I think we should try to find a way to solve https://github.com/playframework/play-json/pull/557/files#r559527225 otherwise it is another pain in the future dotty migration for users.
"EnumFormat" should { | ||
import TestEnums.EnumWithDefaultNames._ | ||
|
||
// https://gitter.im/lampepfl/dotty?at=5ee22d1e7b6da9126a8b4a51 ¯\_(ツ)_/¯ |
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.
Not sure what was hiding behind this link, however with Scala 3 things now work again, see #848
Fixes #539