Initial Duration module #296
Conversation
| import scala.annotation.switch | ||
| import scala.concurrent.duration.Duration as SDuration | ||
|
|
||
| opaque type Duration <: JDuration = JDuration |
There was a problem hiding this comment.
Do you think we could use opaque type Duration = Long? It'd be nice to avoid the allocation of Java's wrapper.
There was a problem hiding this comment.
It's possible, but my goal was to directly 'replace' Java Duration. Can we make it Java duration compatible - ie, a value class that extends Duration?
There was a problem hiding this comment.
I don't think so, value classes can only extend "universal traits": https://docs.scala-lang.org/overviews/core/value-classes.html.
Maybe a toJava would be enough to provide interop?
| private[kyo] inline def isFinite: Boolean = self < Duration.Infinity | ||
| end extension | ||
|
|
||
| private[kyo] object duration: |
There was a problem hiding this comment.
I wasn't able to find a different way to protect internal implementation details from the rest of the kyo codebase. This leads to some duplication, but it doesn't seem too bad.
There was a problem hiding this comment.
What part of the implementation did you want to avoid exposing here? Does anything fail if you move the opaque type to line 10?
There was a problem hiding this comment.
The issue is that internal aspects of Kyo's code base become Duration (Longs, Chars, etc) meaning the functions like to are tied to Duration:
This line will cause a failure to compile
https://github.com/getkyo/kyo/blob/main/kyo-core/shared/src/main/scala/kyo/randoms.scala#L54
There was a problem hiding this comment.
That's odd, I think the opaque type should be visible only in the same file.
There was a problem hiding this comment.
oh I see, the issue is the extension methods. Wouldn't that be a problem for users as well?
There was a problem hiding this comment.
The problem was caused by a combination of
extension (sel
10000
f: Duration)
and that
Duration was known throughout the kyo namespace to be a Long. Thus, all Longs got access to internal functions of Durations.
As it stands today, this is not a problem because the Duration type is within its own kyo.duration namespace now.
| // assertTrue(i.days.toDays == i), | ||
| // assertTrue(i.weeks.toWeeks == i), | ||
| // assertTrue(i.months.toMonths == i) |
There was a problem hiding this comment.
We should provide a clear warning regarding the maximum precision that this can contain. It's more than reasonable for nearly all scheduling tasks...
There was a problem hiding this comment.
Maybe we could fail in case of overflow? Another option is bitwise operations/masks to represent different granularities of durations using a Long but I'm not sure we need that.
There was a problem hiding this comment.
Let's proceed with the current module, we can get some feedback from users on if they expect exceptions for overflow. I'm concerned with the safety of random exceptions - but also not excited about silent overflow.
| @@ -368,7 +368,6 @@ While the companion object of `KyoApp` provides utility methods to run isolated | |||
|
|
|||
| ```scala | |||
| import kyo._ | |||
| import scala.concurrent.duration._ | |||
There was a problem hiding this comment.
Not requiring this import is a nice improvement to!
| inline def toYears: Long = self.to(Years) | ||
|
|
||
| def toScala: ScalaDuration = | ||
| (self: @annotation.switch) match |
There was a problem hiding this comment.
cool, I didn't know this annotation!
There was a problem hiding this comment.
it directly compiles match to a java switch. It's a good way to ensure performance, even if the compiler should do the right thing without it. It should fail compilation where the compilation is not possible.
| import scala.concurrent.duration.Duration as ScalaDuration | ||
|
|
||
| type Duration = duration.Duration | ||
| given CanEqual[Duration, Duration] = canEqual |
There was a problem hiding this comment.
Can you move this to the companion object?
There was a problem hiding this comment.
I don't think it's possible. The type is necessary to be in the top level namespace for the extension methods.
| inline def gt(that: Duration): Boolean = self > that | ||
| inline def lt(that: Duration): Boolean = self < that | ||
| inline def eqEq(that: Duration): Boolean = self == that | ||
| inline def neEq(that: Duration): Boolean = self == that |
Needs tests + some minor reworking, but should be usable.
Improves compatibility with ZIO interop and general API interop