lloydmeta / enumeratum

A type-safe, reflection-free, powerful enumeration implementation for Scala with exhaustive pattern match warnings and helpful integrations.
MIT License
1.18k stars 146 forks source link

🏗️ build(play-json): upgrade to Play JSON 3.0 #380

Closed gaeljw closed 7 months ago

gaeljw commented 8 months ago

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: https://github.com/playframework/play-json/compare/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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

Codecov Report

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

:exclamation: 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 commented 8 months ago

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.

https://github.com/lloydmeta/enumeratum/blob/dba1630b5eedfb49811c3f28fa8ee060e0fc78df/build.sbt#L357

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?

https://github.com/lloydmeta/enumeratum/blob/dba1630b5eedfb49811c3f28fa8ee060e0fc78df/build.sbt#L141

I wonder if it can be made optional.

gaeljw commented 8 months ago

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 commented 7 months ago

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