Update Scalafix migration for Cats 2.2.0

[fthomas]

See #1588

[codecov]

Codecov Report

Merging #1594 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1594   +/-   ##
=======================================
  Coverage   70.28%   70.28%           
=======================================
  Files         113      113           
  Lines        1780     1780           
  Branches       48       48           
=======================================
  Hits         1251     1251           
  Misses        529      529           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b90a32...4ecf1a8. Read the comment docs.

[cb372]

Should this be updated to "2.2.0"?

[mzuehlke]

The current value for newVersion would trigger the execution of the migration whenever the new version is bigger than 2.2.0-RC4.As tried out by @fthomas this would bump users from 2.2.0-RC3 to 2.2.0-RC4 using the migration.

Projects on 2.1.1 would get upgraded to 2.2.0 using the migration once it is available without further action required. And if there would happen a 2.2.0-RC5 before users already on 2.2.0-RC4 wouldn't get the migration 2nd time.

[fthomas]

@mzuehlke is correct. - The only downside of not updating newVersion is that projects, that were already on 2.2.0-RC4 before the migration was added here, won't get the migration with the bump to 2.2.0.

Since it seems that I'm the only one who tested the migration with the public Scala Steward instance so far (see https://github.com/search?q=author%3Ascala-steward+is%3Apr+cats+2.2.0-RC4+Applied+Migrations) it might make sense to bump newVersion to 2.2.0 nonetheless. I don't mind if Scala Steward applies the migration a 2nd time on my projects.

[fthomas]

2.2.0 will probably be released today (https://gitter.im/typelevel/cats-dev?at=5f50d85d59ac794e02e56b5b) so I think it makes sense to bump newVersion too.

[fthomas]

Bumped in #1596.

[mzuehlke]

👍

[fthomas]

PRs with this Scalafix migration can be found here: https://github.com/pulls?q=author%3Ascala-steward+is%3Apr+github:typelevel/cats/Cats_v2_2_0

[cb372]

I see a few weird PRs where scala-steward appears to be updating the wrong dependency, e.g. https://github.com/evolution-gaming/sharding-strategy/pull/48/files and https://github.com/evolution-gaming/kafka-flow/pull/68/files. Is this a known issue?

Here it's not updating any dependencies at all, but it runs the scalafix rule: https://github.com/evolution-gaming/pubsub/pull/66/files

Regarding the rule itself, unfortunately it looks like there are still a couple of cases we missed in our testing. e.g. chenharryhua/nanjin#712 doesn't appear to have CI set up, but I cloned it and tried compiling the PR. There are a few compilation errors of the form:

[error] /Users/chris/code/nanjin/datetime/src/main/scala/com/github/chenharryhua/nanjin/datetime/NJDateTimeRange.scala:48:36: value mapN is not a member of (Option[java.time.ZonedDateTime], Option[java.time.ZonedDateTime])
[error]     (zonedStartTime, zonedEndTime).mapN((s, e) => Period.between(s.toLocalDate, e.toLocalDate))

That's a shame, but hopefully the rule is still useful for the majority of people.

[fthomas]

I see a few weird PRs where scala-steward appears to be updating the wrong dependency, e.g. https://github.com/evolution-gaming/sharding-strategy/pull/48/files and https://github.com/evolution-gaming/kafka-flow/pull/68/files. Is this a known issue?

Yes, that is tracked in #1184.

Here it's not updating any dependencies at all, but it runs the scalafix rule: https://github.com/evolution-gaming/pubsub/pull/66/files

That can happen too. Normally Scala Steward just does not open a PR if it can't find the version number to update. But if a Scalafix migration causes changes it creates a PR anyway. Maybe it shouldn't do that.

Regarding the rule itself, unfortunately it looks like there are still a couple of cases we missed in our testing.

The majority of PRs have now been merged as-is, so I consider this a success.