Skip to content
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

πŸ—οΈ build(play-json): upgrade to Play JSON 3.0 #380

Closed
wants to merge 1 commit into from

Conversation

gaeljw
Copy link
Contributor

@gaeljw gaeljw commented Nov 3, 2023

Bump play-json dependency in enumeratum-play-json module to 3.0.

Play-JSON 3.0 is strictly the same in terms of code compared to 2.10.1 (see the diff: playframework/play-json@2.10.1...3.0.0). The major change is due to the groupId renaming from com.typesafe.play to org.playframework.


Note that I didn't work on upgrading to Play 3.0 in enumeratum-play for now as there's a bit more work but I'd happy to give it a try in the coming days.

I'm not entirely sure how to go about it though, as this may force downstream users to upgrade (mainly because of the underlying change from Akka to Pekko in Play). I would need to check if enumeratum can be compatible with both Play 2 and Play 3.

I guess I'll provide another PR and then we can discuss and maybe test compatibility on real projects (I have some at my company that can be good candidates).

@gaeljw
Copy link
Contributor Author

gaeljw commented Nov 3, 2023

Also I didn't find any changelog, this change is worth mentioning with maybe an extra word because downstream users will have to choose in their dependency tree if they keep using com.typesafe.play:play-json or they move to org.playframework:play-json. As if they do nothing, they could have both in the classpath. Which shouldn't be an issue because the code is exactly the same but still this might bring confusion.

@gaeljw
Copy link
Contributor Author

gaeljw commented Nov 3, 2023

@lloydmeta one last thing, I was not able to run tests locally on my machine when running following commands:

sbt
> project play-json-aggregate
> test

It fails with some compilation errors that I couldn't understand what to do to avoid them:

[error] /home/xxx/sources/github/enumeratum/enumeratum-play-json/src/test/scala/enumeratum/values/EnumFormatsSpec.scala:16:33: not found: value LibraryItem
[error] Error occurred in an application involving default arguments.
[error]     testNumericReads("IntEnum", LibraryItem)
[error]                                 ^

What am I missing?

@codecov-commenter
Copy link

Codecov Report

Merging #380 (cb7da33) into master (dba1630) will not change coverage.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##           master     #380   +/-   ##
=======================================
  Coverage   85.52%   85.52%           
=======================================
  Files          65       65           
  Lines         518      518           
  Branches       31       31           
=======================================
  Hits          443      443           
  Misses         75       75           

@lloydmeta
Copy link
Owner

Note that I didn't work on upgrading to Play 3.0 in enumeratum-play for now as there's a bit more work but I'd happy to give it a try in the coming days.

I'm not entirely sure how to go about it though, as this may force downstream users to upgrade (mainly because of the underlying change from Akka to Pekko in Play). I would need to check if enumeratum can be compatible with both Play 2 and Play 3.

I guess I'll provide another PR and then we can discuss and maybe test compatibility on real projects (I have some at my company that can be good candidates).

I think what possibly complicates things (more!) is that the enumeratum-play module has a dependency on enumeratum-play-json.

.dependsOn(enumeratumPlayJsonJvm % "compile->compile;test->test")

To be honest, at this point, I would not be sad if we dropped all support going forward for Play < 3 in favour of just Play 3+ . Having read the doc on the Play website, moving ahead with Pekko-based Play and encouraging everyone to do so feels like something that makes sense. So, I would say, if it simplifies a lot of things to just do this in order to move enumeratum-play to Play 3, let's just drop support for older versions.

That said, if you're (or anyone reading this) strongly opinionated against this, please shout.

@lloydmeta one last thing, I was not able to run tests locally on my machine when running following commands:
...
What am I missing?

I think you might be missing passing -Denumeratum.useLocalVersion as a sys prop?

lazy val useLocalVersion = sys.props.get("enumeratum.useLocalVersion").nonEmpty

I wonder if it can be made optional.

@gaeljw
Copy link
Contributor Author

gaeljw commented Nov 4, 2023

To be honest, at this point, I would not be sad if we dropped all support going forward for Play < 3 in favour of just Play 3+ . Having read the doc on the Play website, moving ahead with Pekko-based Play and encouraging everyone to do so feels like something that makes sense. So, I would say, if it simplifies a lot of things to just do this in order to move enumeratum-play to Play 3, let's just drop support for older versions.

Makes sense to me as well. It's what Tapir did.

And I guess that if there's demand for Play < 3 compatibility, it could still be made available with a dedicated module. But I would tend to avoid it in the 1st place.


Thanks for the info, I'll try to work on Play 3 today or tomorrow.

@lloydmeta
Copy link
Owner

I believe this can be closed since #381 has been merged.

@lloydmeta lloydmeta closed this Nov 12, 2023
@gaeljw gaeljw deleted the playjson3 branch November 12, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants