Skip to content

Orthogonal DateTimePeriod implementation #80

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Feb 25, 2021
Merged
309 changes: 250 additions & 59 deletions core/common/src/DateTimePeriod.kt

Large diffs are not rendered by default.

18 changes: 8 additions & 10 deletions core/common/src/Instant.kt
Original file line number Diff line number Diff line change
Expand Up @@ -192,19 +192,15 @@ public fun Instant.minus(period: DateTimePeriod, timeZone: TimeZone): Instant =
/* An overflow can happen for any component, but we are only worried about nanoseconds, as having an overflow in
any other component means that `plus` will throw due to the minimum value of the numeric type overflowing the
platform-specific limits. */
if (period.nanoseconds != Long.MIN_VALUE) {
val negatedPeriod = with(period) {
DateTimePeriod(-years, -months, -days, -hours, -minutes, -seconds, -nanoseconds)
}
if (period.totalNanoseconds != Long.MIN_VALUE) {
val negatedPeriod = with(period) { buildDateTimePeriod(-totalMonths, -days, -totalNanoseconds) }
plus(negatedPeriod, timeZone)
} else {
val negatedPeriod = with(period) {
DateTimePeriod(-years, -months, -days, -hours, -minutes, -seconds, -(nanoseconds+1))
}
val negatedPeriod = with(period) { buildDateTimePeriod(-totalMonths, -days, -(totalNanoseconds+1)) }
plus(negatedPeriod, timeZone).plus(DateTimeUnit.NANOSECOND)
}

/**
/**
* Returns a [DateTimePeriod] representing the difference between `this` and [other] instants.
*
* The components of [DateTimePeriod] are calculated so that adding it to `this` instant results in the [other] instant.
Expand All @@ -214,7 +210,8 @@ public fun Instant.minus(period: DateTimePeriod, timeZone: TimeZone): Instant =
* - negative or zero if this instant is later than the other,
* - exactly zero if this instant is equal to the other.
*
* @throws DateTimeArithmeticException if `this` or [other] instant is too large to fit in [LocalDateTime].
* @throws DateTimeArithmeticException if `this` or [other] instant is too large to fit in [LocalDateTime]. Also (only
* on JVM) if the number of months between the two dates exceeds an Int.
*/
public expect fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateTimePeriod

Expand Down Expand Up @@ -296,7 +293,8 @@ public fun Instant.yearsUntil(other: Instant, timeZone: TimeZone): Int =
* - positive or zero if this instant is later than the other,
* - exactly zero if this instant is equal to the other.
*
* @throws DateTimeArithmeticException if `this` or [other] instant is too large to fit in [LocalDateTime].
* @throws DateTimeArithmeticException if `this` or [other] instant is too large to fit in [LocalDateTime]. Also (only
* on JVM) if the number of months between the two dates exceeds an Int.
* @see Instant.periodUntil
*/
public fun Instant.minus(other: Instant, timeZone: TimeZone): DateTimePeriod =
Expand Down
4 changes: 4 additions & 0 deletions core/common/src/LocalDate.kt
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ public operator fun LocalDate.minus(period: DatePeriod): LocalDate =
* - negative or zero if this date is later than the other,
* - exactly zero if this date is equal to the other.
*
* @throws DateTimeArithmeticException if the number of months between the two dates exceeds an Int (JVM only).
*
* @see LocalDate.minus
*/
expect fun LocalDate.periodUntil(other: LocalDate): DatePeriod
Expand All @@ -151,6 +153,8 @@ expect fun LocalDate.periodUntil(other: LocalDate): DatePeriod
* - positive or zero if this date is later than the other,
* - exactly zero if this date is equal to the other.
*
* @throws DateTimeArithmeticException if the number of months between the two dates exceeds an Int (JVM only).
*
* @see LocalDate.periodUntil
*/
operator fun LocalDate.minus(other: LocalDate): DatePeriod = other.periodUntil(this)
Expand Down
19 changes: 19 additions & 0 deletions core/common/src/math.kt
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,22 @@ internal fun multiplyAddAndDivide(d: Long, n: Long, r: Long, m: Long): Long {
val (rd, rr) = multiplyAndDivide(md, n, m)
return safeAdd(rd, safeAdd(mr / m, safeAdd(mr % m, rr) / m))
}

/**
* Calculates [d] * [n] + [r], where [n] > 0 and |[r]| <= [n].
*
* @throws ArithmeticException if the result overflows a long
*/
internal fun multiplyAndAdd(d: Long, n: Long, r: Long): Long {
var md = d
var mr = r
// make sure [md] and [mr] have the same sign
if (d > 0 && r < 0) {
md--
mr += n
} else if (d < 0 && r > 0) {
md++
mr -= n
}
return safeAdd(safeMultiply(md, n), mr)
}
117 changes: 114 additions & 3 deletions core/common/test/DateTimePeriodTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,45 @@ import kotlin.time.*

class DateTimePeriodTest {

@Test
fun normalization() {
assertPeriodComponents(DateTimePeriod(years = 1) as DatePeriod, years = 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use this assertion function in tests for parse as well?

Copy link
Collaborator Author

@dkhalanskyjb dkhalanskyjb Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but, in my opinion, it would make those tests less clear. For example,

         assertEquals(DateTimePeriod(months = 14), DateTimePeriod.parse("P14M"))

clearly implies that DateTimePeriod(months = m) == DateTimePeriod.parse("P${m}M"), which is a property that we want. Seeing the result in terms of values of class properties would obscure this. Moreover, the values of class properties are now being tested with normalization().

What benefits do you see in using this function for tests of parsing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By mixing parsing and normalized equality this test makes an impression that the resulting properties of the parsed DateTimePeriod would be the same as those passed to the constructor-like function of expected value. This is misleading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some unfathomable reason, I didn't see a notification about your response. I am sorry for the delay.

I still disagree and consider these concerns orthogonal: we want parsing to behave the same way construction does, and we want construction to produce specific values of properties. Parsing is not semantically connected to the values of properties, and mixing these rubs me the wrong way. However, I don't consider this issue important enough to die on this hill, so I added a commit to accommodate your preferences, though I would be glad to revert it were you to change your mind about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a compromise, we could leave the equality assertions in the parsing test, but add assertPeriodComponents for those values that cause normalization to happen. This would be done mostly for explanatory reasons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added checks for actual values of properties to places where normalization does affect them.

assertPeriodComponents(DateTimePeriod(years = 1, months = 1) as DatePeriod, years = 1, months = 1)
assertPeriodComponents(DateTimePeriod(years = 1, months = -1) as DatePeriod, months = 11)
assertPeriodComponents(DateTimePeriod(years = -1, months = 1) as DatePeriod, months = -11)
assertPeriodComponents(DateTimePeriod(years = -1, months = -1) as DatePeriod, years = -1, months = -1)
assertPeriodComponents(DateTimePeriod(months = 11) as DatePeriod, months = 11)
assertPeriodComponents(DateTimePeriod(months = 14) as DatePeriod, years = 1, months = 2)
assertPeriodComponents(DateTimePeriod(months = -14) as DatePeriod, years = -1, months = -2)
assertPeriodComponents(DateTimePeriod(months = 10, days = 5) as DatePeriod, months = 10, days = 5)
assertPeriodComponents(DateTimePeriod(years = 1, days = 40) as DatePeriod, years = 1, days = 40)
assertPeriodComponents(DateTimePeriod(years = 1, days = -40) as DatePeriod, years = 1, days = -40)
assertPeriodComponents(DateTimePeriod(days = 5) as DatePeriod, days = 5)

assertPeriodComponents(DateTimePeriod(hours = 3), hours = 3)
assertPeriodComponents(DateTimePeriod(hours = 1, minutes = 120), hours = 3)
assertPeriodComponents(DateTimePeriod(hours = 1, minutes = 119, seconds = 60), hours = 3)
assertPeriodComponents(DateTimePeriod(hours = 1, minutes = 119, seconds = 59, nanoseconds = 1_000_000_000), hours = 3)
assertPeriodComponents(DateTimePeriod(hours = 1, minutes = 121, seconds = -59, nanoseconds = -1_000_000_000), hours = 3)
assertPeriodComponents(DateTimePeriod())
assertPeriodComponents(DatePeriod())

assertPeriodComponents(DateTimePeriod(days = 1, hours = -1), days = 1, hours = -1)
assertPeriodComponents(DateTimePeriod(days = -1, hours = -1), days = -1, hours = -1)

assertPeriodComponents(DateTimePeriod(years = -1, months = -2, days = -3, hours = -4, minutes = -5, seconds = 0, nanoseconds = 500_000_000),
years = -1, months = -2, days = -3, hours = -4, minutes = -4, seconds = -59, nanoseconds = -500_000_000)

assertPeriodComponents(DateTimePeriod(nanoseconds = 999_999_999_999_999L), hours = 277, minutes = 46, seconds = 39, nanoseconds = 999_999_999)
assertPeriodComponents(DateTimePeriod(nanoseconds = -999_999_999_999_999L), hours = -277, minutes = -46, seconds = -39, nanoseconds = -999_999_999)
}

@Test
fun toStringConversion() {
assertEquals("P1Y", DateTimePeriod(years = 1).toString())
assertEquals("P1Y1M", DatePeriod(years = 1, months = 1).toString())
assertEquals("P11M", DateTimePeriod(months = 11).toString())
assertEquals("P14M", DateTimePeriod(months = 14).toString()) // TODO: normalize or not
assertEquals("P1Y2M", DateTimePeriod(months = 14).toString())
assertEquals("P10M5D", DateTimePeriod(months = 10, days = 5).toString())
assertEquals("P1Y40D", DateTimePeriod(years = 1, days = 40).toString())

Expand All @@ -29,8 +62,74 @@ class DateTimePeriodTest {
assertEquals("-P1DT1H", DateTimePeriod(days = -1, hours = -1).toString())
assertEquals("-P1M", DateTimePeriod(months = -1).toString())

assertEquals("P-1Y-2M-3DT-4H-5M0.500000000S",
assertEquals("-P1Y2M3DT4H4M59.500000000S",
DateTimePeriod(years = -1, months = -2, days = -3, hours = -4, minutes = -5, seconds = 0, nanoseconds = 500_000_000).toString())

assertEquals("PT277H46M39.999999999S", DateTimePeriod(nanoseconds = 999_999_999_999_999L).toString())
assertEquals("PT0.999999999S", DateTimePeriod(seconds = 1, nanoseconds = -1L).toString())
assertEquals("-PT0.000000001S", DateTimePeriod(nanoseconds = -1L).toString())
assertEquals("P1DT-0.000000001S", DateTimePeriod(days = 1, nanoseconds = -1L).toString())
assertEquals("-PT0.999999999S", DateTimePeriod(seconds = -1, nanoseconds = 1L).toString())
assertEquals("P1DT-0.999999999S", DateTimePeriod(days = 1, seconds = -1, nanoseconds = 1L).toString())
}

@Test
fun parseIsoString() {
assertEquals(DateTimePeriod(years = 1), DateTimePeriod.parse("P1Y"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to assert the expected property values rather than equality as a whole in this test. Because otherwise it's unclear what values these properties would get as a result of parsing.

Copy link
Collaborator Author

@dkhalanskyjb dkhalanskyjb Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You raise a good point, though I think that the values of properties and the results of parsing are separate concerns, so I added another test to check the normalization.

assertEquals(DatePeriod(years = 1, months = 1), DateTimePeriod.parse("P1Y1M"))
assertEquals(DateTimePeriod(months = 11), DateTimePeriod.parse("P11M"))
assertEquals(DateTimePeriod(months = 10, days = 5), DateTimePeriod.parse("P10M5D"))
assertEquals(DateTimePeriod(years = 1, days = 40), DateTimePeriod.parse("P1Y40D"))

assertEquals(DateTimePeriod(months = 14), DateTimePeriod.parse("P14M"))
assertPeriodComponents(DateTimePeriod.parse("P14M") as DatePeriod, years = 1, months = 2)

assertEquals(DateTimePeriod(hours = 1), DateTimePeriod.parse("PT1H"))
assertEquals(DateTimePeriod(), DateTimePeriod.parse("P0D"))
assertEquals(DatePeriod(), DateTimePeriod.parse("P0D"))

assertEquals(DateTimePeriod(days = 1, hours = -1), DateTimePeriod.parse("P1DT-1H"))
assertEquals(DateTimePeriod(days = -1, hours = -1), DateTimePeriod.parse("-P1DT1H"))
assertEquals(DateTimePeriod(months = -1), DateTimePeriod.parse("-P1M"))

assertEquals(DateTimePeriod(years = -1, months = -2, days = -3, hours = -4, minutes = -5, seconds = 0, nanoseconds = 500_000_000),
DateTimePeriod.parse("P-1Y-2M-3DT-4H-5M0.500000000S"))
assertPeriodComponents(DateTimePeriod.parse("P-1Y-2M-3DT-4H-5M0.500000000S"),
years = -1, months = -2, days = -3, hours = -4, minutes = -4, seconds = -59, nanoseconds = -500_000_000)

assertEquals(DateTimePeriod(nanoseconds = 999_999_999_999_999L), DateTimePeriod.parse("PT277H46M39.999999999S"))
assertPeriodComponents(DateTimePeriod.parse("PT277H46M39.999999999S"),
hours = 277, minutes = 46, seconds = 39, nanoseconds = 999_999_999)

assertEquals(DateTimePeriod(nanoseconds = 999_999_999), DateTimePeriod.parse("PT0.999999999S"))
assertEquals(DateTimePeriod(nanoseconds = -1), DateTimePeriod.parse("-PT0.000000001S"))
assertEquals(DateTimePeriod(days = 1, nanoseconds = -1), DateTimePeriod.parse("P1DT-0.000000001S"))
assertEquals(DateTimePeriod(nanoseconds = -999_999_999), DateTimePeriod.parse("-PT0.999999999S"))
assertEquals(DateTimePeriod(days = 1, nanoseconds = -999_999_999), DateTimePeriod.parse("P1DT-0.999999999S"))
assertPeriodComponents(DateTimePeriod.parse("P1DT-0.999999999S"), days = 1, nanoseconds = -999_999_999)

// overflow of `Int.MAX_VALUE` months
assertFailsWith<IllegalArgumentException> { DateTimePeriod.parse("P2000000000Y") }
assertFailsWith<IllegalArgumentException> { DateTimePeriod.parse("P1Y2147483640M") }

// too large a number in a field
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000Y") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000M") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000D") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000H") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000M") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P3000000000S") }

// wrong order of signifiers
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P1Y2D3M") }
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P0DT1M2H") }

// loss of precision in fractional seconds
assertFailsWith<DateTimeFormatException> { DateTimePeriod.parse("P0.000000000001S") }

// non-zero time components when parsing DatePeriod
assertFailsWith<IllegalArgumentException> { DatePeriod.parse("P1DT1H") }
DatePeriod.parse("P1DT0H")
}

@Test
Expand Down Expand Up @@ -69,4 +168,16 @@ class DateTimePeriodTest {
assertEquals(period, duration.toDateTimePeriod())
}
}
}

private fun assertPeriodComponents(period: DateTimePeriod,
years: Int = 0, months: Int = 0, days: Int = 0,
hours: Int = 0, minutes: Int = 0, seconds: Int = 0, nanoseconds: Int = 0) {
assertEquals(years, period.years)
assertEquals(months, period.months)
assertEquals(days, period.days)
assertEquals(hours, period.hours)
assertEquals(minutes, period.minutes)
assertEquals(seconds, period.seconds)
assertEquals(nanoseconds, period.nanoseconds)
}
}
19 changes: 10 additions & 9 deletions core/common/test/InstantTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -468,9 +468,11 @@ class InstantRangeTest {
fun periodArithmeticOutOfRange() {
// Instant.plus(DateTimePeriod(), TimeZone)
// Arithmetic overflow
for (instant in smallInstants + largeNegativeInstants + largePositiveInstants) {
assertArithmeticFails("$instant") { instant.plus(DateTimePeriod(seconds = Long.MAX_VALUE), UTC) }
assertArithmeticFails("$instant") { instant.plus(DateTimePeriod(seconds = Long.MIN_VALUE), UTC) }
for (instant in largePositiveInstants) {
assertArithmeticFails("$instant") { instant.plus(DateTimePeriod(nanoseconds = Long.MAX_VALUE), UTC) }
}
for (instant in largeNegativeInstants) {
assertArithmeticFails("$instant") { instant.plus(DateTimePeriod(nanoseconds = Long.MIN_VALUE), UTC) }
}
// Arithmetic overflow in an Int
for (instant in smallInstants + listOf(maxValidInstant)) {
Expand All @@ -494,9 +496,8 @@ class InstantRangeTest {
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(nanoseconds = 1), UTC) }
assertArithmeticFails { minValidInstant.plus(DateTimePeriod(nanoseconds = -1), UTC) }
// Overflowing a LocalDateTime in intermediate computations
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(seconds = 1, nanoseconds = -1_000_000_001), UTC) }
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(hours = 1, minutes = -61), UTC) }
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(days = 1, hours = -48), UTC) }
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(days = 1, nanoseconds = -1_000_000_001), UTC) }
assertArithmeticFails { maxValidInstant.plus(DateTimePeriod(months = 1, days = -48), UTC) }
}

@Test
Expand Down Expand Up @@ -540,9 +541,9 @@ class InstantRangeTest {
@Test
fun periodUntilOutOfRange() {
// Instant.periodUntil
maxValidInstant.periodUntil(minValidInstant, UTC)
assertArithmeticFails { (maxValidInstant + 1.nanoseconds).periodUntil(minValidInstant, UTC) }
assertArithmeticFails { maxValidInstant.periodUntil(minValidInstant - 1.nanoseconds, UTC) }
maxValidInstant.periodUntil(maxValidInstant, UTC)
assertArithmeticFails { (maxValidInstant + 1.nanoseconds).periodUntil(maxValidInstant, UTC) }
assertArithmeticFails { minValidInstant.periodUntil(minValidInstant - 1.nanoseconds, UTC) }
}

@Test
Expand Down
13 changes: 5 additions & 8 deletions core/js/src/Instant.kt
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,12 @@ public actual fun Instant.plus(period: DateTimePeriod, timeZone: TimeZone): Inst
val thisZdt = this.value.atZone(timeZone.zoneId)
with(period) {
thisZdt
.run { if (years != 0 && months == 0) plusYears(years) else this }
.run { if (months != 0) plusMonths(years * 12.0 + months) else this }
.run { if (totalMonths != 0) plusMonths(totalMonths) else this }
.run { if (days != 0) plusDays(days) as ZonedDateTime else this }
.run { if (hours != 0) plusHours(hours) else this }
.run { if (minutes != 0) plusMinutes(minutes) else this }
.run { if (seconds != 0L) plusSeconds(seconds.toDouble()) else this }
.run { if (nanoseconds != 0L) plusNanos(nanoseconds.toDouble()) else this }
.run { if (seconds != 0) plusSeconds(seconds) else this }
.run { if (nanoseconds != 0) plusNanos(nanoseconds.toDouble()) else this }
}.toInstant().let(::Instant)
} catch (e: Throwable) {
if (e.isJodaDateTimeException()) throw DateTimeArithmeticException(e)
Expand Down Expand Up @@ -188,11 +187,9 @@ public actual fun Instant.periodUntil(other: Instant, timeZone: TimeZone): DateT

val months = thisZdt.until(otherZdt, ChronoUnit.MONTHS).toDouble(); thisZdt = thisZdt.plusMonths(months)
val days = thisZdt.until(otherZdt, ChronoUnit.DAYS).toDouble(); thisZdt = thisZdt.plusDays(days) as ZonedDateTime
val time = thisZdt.until(otherZdt, ChronoUnit.NANOS).toDouble().nanoseconds
val nanoseconds = thisZdt.until(otherZdt, ChronoUnit.NANOS).toDouble()

time.toComponents { hours, minutes, seconds, nanoseconds ->
return DateTimePeriod((months / 12).toInt(), (months % 12).toInt(), days.toInt(), hours, minutes, seconds.toLong(), nanoseconds.toLong())
}
buildDateTimePeriod(months.toInt(), days.toInt(), nanoseconds.toLong())
} catch (e: Throwable) {
if (e.isJodaDateTimeException()) throw DateTimeArithmeticException(e) else throw e
}
Expand Down
5 changes: 2 additions & 3 deletions core/js/src/LocalDate.kt
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ private fun LocalDate.plusNumber(value: Number, unit: DateTimeUnit.DateBased): L
public actual operator fun LocalDate.plus(period: DatePeriod): LocalDate = try {
with(period) {
return@with value
.run { if (years != 0 && months == 0) plusYears(years) else this }
.run { if (months != 0) plusMonths(years.toDouble() * 12 + months) else this }
.run { if (totalMonths != 0) plusMonths(totalMonths) else this }
.run { if (days != 0) plusDays(days) else this }

}.let(::LocalDate)
Expand All @@ -86,7 +85,7 @@ public actual fun LocalDate.periodUntil(other: LocalDate): DatePeriod {
val months = startD.until(endD, ChronoUnit.MONTHS).toInt(); startD = startD.plusMonths(months)
val days = startD.until(endD, ChronoUnit.DAYS).toInt()

return DatePeriod(months / 12, months % 12, days)
return DatePeriod(totalMonths = months, days)
}

public actual fun LocalDate.until(other: LocalDate, unit: DateTimeUnit.DateBased): Int = when(unit) {
Expand Down
Loading