twitter / util

Wonderful reusable code from Twitter
https://twitter.github.io/util
Apache License 2.0
2.68k stars 580 forks source link

util/util-core: Fix conversions to units from Int in Scala3 #296

Closed felixbr closed 2 years ago

felixbr commented 2 years ago

This fixes lost source-compatibility for unit conversions from Int values in Scala3. As discussed in #295 I extracted/adapted this from #290 (previous discussion).

Previously something like 1.toInt.second did compile in Scala2 (with default settings) but didn't in Scala3.

I added forwarders for all units which use Long as base, as that seemed reasonable and safe. I didn't add an implicit conversion from Float to Double because floating-point types are dumb and I don't trust the conversion to be lossless in all cases.

I'll adapt #295 once this is merged.

felixbr commented 2 years ago

Would it make sense to update Scala from 3.0.2-RC1 to 3.0.2 and add it to CI? This would help with avoiding regressions like this in the future. Currently PRs are not automatically tested against Scala 3.

codecov-commenter commented 2 years ago

Codecov Report

Merging #296 (7e2687e) into develop (2d5817f) will decrease coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #296      +/-   ##
===========================================
- Coverage    52.62%   52.62%   -0.01%     
===========================================
  Files          316      316              
  Lines        16856    16859       +3     
  Branches      1010     1009       -1     
===========================================
+ Hits          8871     8872       +1     
- Misses        7985     7987       +2     
Impacted Files Coverage Δ
...in/scala/com/twitter/conversions/DurationOps.scala 100.00% <100.00%> (ø)
...scala/com/twitter/conversions/StorageUnitOps.scala 81.25% <100.00%> (+1.25%) :arrow_up:
...rc/main/scala/com/twitter/conversions/U64Ops.scala 100.00% <100.00%> (ø)
...twitter/finagle/stats/BroadcastStatsReceiver.scala 36.48% <0.00%> (-1.36%) :arrow_down:
...core/src/main/scala/com/twitter/util/Promise.scala 77.73% <0.00%> (-0.81%) :arrow_down:
...ore/src/main/scala/com/twitter/util/Duration.scala 85.39% <0.00%> (+0.56%) :arrow_up:

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 2d5817f...7e2687e. Read the comment docs.

hamdiallam commented 2 years ago

Would it make sense to update Scala from 3.0.2-RC1 to 3.0.2 and add it to CI? This would help with avoiding regressions like this in the future. Currently PRs are not automatically tested against Scala 3.

Yes, let's update to 3.02! Was waiting for that release which includes a few bug fixes which first pushed us to using RC1. I'm for including it in CI but I wonder if we need the entire repo crossbuilding before that could work?

Now that we have forwarders. If it's not a hassle, is it possible to undo the toLong conversions the interns added in the projects that crossbuild scala 3 to originally fix this problem. Otherwise LGTM! I can undo them in a separate pr otherwise

felixbr commented 2 years ago

I'm for including it in CI but I wonder if we need the entire repo crossbuilding before that could work?

One way to do it is to have a separate aggregate-module or even just an alias which tests only the modules which currently support Scala3.

If it's not a hassle, is it possible to undo the toLong conversions the interns added in the projects that crossbuild scala 3 to originally fix this problem.

I did a quick search when I wrote this PR and didn't see any obvious parts where .toLong was added for backwards compat but I might have missed something. Once this is merged, I will undo it in my other PR and rebase onto develop.

yufangong commented 2 years ago

Hey @felixbr, going to merge this and #295 in, is there an order you prefer which one got merged first, looks like we need to adapt them at the end. Thanks!

felixbr commented 2 years ago

The idea was to merge this first and rebase the other one, so the workaround there isn't needed anymore.

I can do the rebase and adaptation once this is merged, if you want.

yufangong commented 2 years ago

Sorry for the delay, this is merged at https://github.com/twitter/util/commit/ad8b22b9adbe1958f33b08cf133a9baac2c0aa9a, let us know when the #295 is ready. thank you!