Statically distinguish date-only and date+time units and periods #7
Conversation
ilya-g
force-pushed
the
master
branch
2 times, most recently
from
eb9fdee to
a1cc52d
Compare
ilya-g
force-pushed
the
date-calc
branch
2 times, most recently
from
163b762 to
63c8395
Compare
ilya-g
changed the title
... for consistency with LocalDate operations.
ilya-g
requested review from
dkhalanskyjb and
kluever
and removed request for
dkhalanskyjb and
kluever
|
Hi, @kluever, you wanted to review the new approach to date/time units. Sorry, I cannot request a review from more than one reviewer for this PR for some reason, so I'm just notifying you with this message. |
| public fun LocalDate.plus(unit: DateTimeUnit.DateBased): LocalDate = | ||
| plus(unit.calendarScale, unit.calendarUnit) | ||
| public fun LocalDate.plus(value: Int, unit: DateTimeUnit.DateBased): LocalDate = | ||
| plus(value * unit.calendarScale, unit.calendarUnit) |
There was a problem hiding this comment.
Here, too, safeMultiply or the like is needed.
There was a problem hiding this comment.
Will be used later, after commonizing that function.
| * @throws DateTimeArithmeticException if this value or the result is too large to fit in [LocalDateTime]. | ||
| */ | ||
| public fun Instant.plus(value: Int, unit: DateTimeUnit, zone: TimeZone): Instant = | ||
| plus(value * unit.calendarScale, unit.calendarUnit, zone) |
There was a problem hiding this comment.
It is possible to provide an example of how this method could throw (assuming safeAdd) even when the result and the intermediate value fits in LocalDateTime:
Clock.System.now().plus(Long.MAX_VALUE, DateTimeUnit.TimeBased(2), TimeZone.UTC)
In effect, we're asking the system to add (2^63 - 1) * 2 nanoseconds, which is less than 600 years: https://www.wolframalpha.com/input/?i=%282%5E63+-+1%29+*+2+nanoseconds+in+years. The result would fit in a LocalDateTime here.
There was a problem hiding this comment.
Perhaps we can specialize plus implementation later for different platforms for better handling this situation.
| try { | ||
| toZonedLocalDateTimeFailing(zone).until(other.toZonedLocalDateTimeFailing(zone), unit) | ||
| // TODO: inline 'until' here and simplify operation for time-based units |
There was a problem hiding this comment.
Avoid converting them to different offsets and then bringing back to the same offset.
Alternatively, ensure that they can be both converted to localdatetime, then calculate difference with Instant components.
| MILLISECOND, | ||
| MICROSECOND, | ||
| NANOSECOND | ||
| } |
There was a problem hiding this comment.
I think splitting CalendarUnit into a similar date/time hierarchy of sealed classes here could simplify some of our implementation code, but I don't know whether this approach has some other downsides, performance etc.
There was a problem hiding this comment.
Better to migrate our implementation code to DateTimeUnit. CalendarUnit is left mostly for conversion to ChronoUnit when interfacing with external implementation.
When 0L was passed as JS number argument, it failed to equal strictly to zero number, thus triggering an incorrect execution path.
Drop WEEK and introduce MILLISECOND and MICROSECOND
In this PR we're removing
CalendarPeriodand introducingDateTimePeriodinstead with its subclassDatePeriod.Also,
CalendarUnitenum with the fixed set of values is replaced withDateTimeUnitsealed hierarchy with several predefined units declared in its companion and an operation to produce derived units which are multiples of the basic units.This allows us to avoid
IllegalArgumentExceptionentirely by prohibiting to use time-based units inLocalDateoperations.