Skip to content
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

Consider deprecating DateTimePeriod.plus and DatePeriod.plus #381

Open
dkhalanskyjb opened this issue Apr 12, 2024 · 4 comments
Open

Consider deprecating DateTimePeriod.plus and DatePeriod.plus #381

dkhalanskyjb opened this issue Apr 12, 2024 · 4 comments
Labels
breaking change This could break existing code
Milestone

Comments

@dkhalanskyjb
Copy link
Collaborator

The purpose of DatePeriod and DateTimePeriod is to define the duration between two dates or instants, respectively. Intuitively, it makes sense to define addition of two periods: if there's time p1 between moments A and B and time p2 between moments B and C, then p1 + p2 could be the amount of time between A and C. Unfortunately, it doesn't work this way.

val date1 = LocalDate(2024, 4, 12)
val date2 = LocalDate(2024, 9, 4)
val date3 = LocalDate(2024, 12, 27)
assertEquals(LocalDate(2024, 12, 27), (date1 + (date2 - date1)) + (date3 - date2)) // without using period addition
assertEquals(LocalDate(2024, 12, 28), date1 + ((date2 - date1) + (date3 - date2))) // using period addition

assertEquals(DatePeriod(months = 4, days = 23), date2 - date1)
assertEquals(DatePeriod(months = 3, days = 23), date3 - date2)
assertEquals(DatePeriod(months = 3, days = 23), date3 - date2)
assertEquals(DatePeriod(months = 8, days = 15), date3 - date1)
assertEquals(DatePeriod(months = 7, days = 46), (date3 - date2) + (date2 - date1))

If we don't add periods together, we get what we expected: x + (y - x) becomes y, so in the end, we get back to date3.

The reason the result is different when we add periods together first lies in the order in which addition of periods happens. In the normal case, we get

date1.plus(4, DateTimeUnit.MONTH).plus(23, DateTimeUnit.DAY).plus(3, DateTimeUnit.MONTH).plus(23, DateTimeUnit.DAY)

In the case of adding periods together first, instead, we get

date1.plus(4, DateTimeUnit.MONTH).plus(3, DateTimeUnit.MONTH).plus(23, DateTimeUnit.DAY).plus(23, DateTimeUnit.DAY)

But waiting for three months and then for 23 days is not the same as waiting for 23 days and then waiting for three months, because the lengths of months are different.

With DateTimePeriod, the situation is even worse, because the time component is also added, giving us additional problems like the difference between waiting for 24 hours and one whole day (which isn't always the same, because clocks could shift due to DST transitions).

With all of this in mind, it's unclear what use cases adding periods together correctly achieves and even what precise semantics it has. If we learn of use cases where plus always does what's expected, we could try to limit plus to just these cases.

@dkhalanskyjb dkhalanskyjb added the breaking change This could break existing code label Apr 12, 2024
@dkhalanskyjb dkhalanskyjb added this to the 0.9.0 milestone Apr 12, 2024
@PeterAttardo
Copy link

I don't think the issue is with DateTimePeriod.plus and DatePeriod.plus exactly. Date{Time}Period addition is both commutative and associative, and the following tests will pass:

@Test
    fun associativeAndCommutative() {
        val p1 = DateTimePeriod(years = 1, months = 1, days = 28, hours = 1, minutes = 1, seconds = 1, nanoseconds = 86399999999997)
        val p2 = DateTimePeriod(years = 2, months = 2, days = 30, hours = 2, minutes = 2, seconds = 2, nanoseconds = 86399999999998)
        val p3 = DateTimePeriod(years = 3, months = 3, days = 31, hours = 3, minutes = 3, seconds = 3, nanoseconds = 86399999999999)

        val dp1 = DatePeriod(years = 1, months = 1, days = 28)
        val dp2 = DatePeriod(years = 2, months = 2, days = 30)
        val dp3 = DatePeriod(years = 3, months = 3, days = 31)

        assertEquals(p1 + p2, p2 + p1)
        assertEquals(dp1 + dp2, dp2 + dp1)
        assertEquals((p1 + p2) + p3, p1 + (p2 + p3))
        assertEquals((dp1 + dp2) + dp3, dp1 + (dp2 + dp3))
    }

The issues arises when adding LocalDate{Time} and Date{Time}Period. Addition between these types is not associative (although it theoretically could be commutative, but currently lacks Date{Time}Period.plus(other: LocalDate{Time})). However, given that adding periods to dates is pretty much the entire point, it wouldn't make sense to deprecate this arithmetic.

It is somewhat reminiscent of when the limitations of numeric types yield different behavior than pure math, such as the following failing tests:

assertEquals(1 / 2 * 2, 1) // integer division means 1/2 rounds down to 0 instead of 0.5
assertEquals<Int>(1 + 1.0 - 1.0, 1) // will not even compile because int/double arithmetic returns a double
assertEquals(0.1 + 0.2, 0.3) // floating point arithmetic simply isn't precise

Even without Date{Time}Period.plus the issues remain. For example the following:

    @Test
    fun addAndSubtract() {
        val ld1 = LocalDate(2000, 1, 31)
        val dp1 = DatePeriod(months = 1)

        assertEquals(ld1 + dp1 - dp1, ld1)
    }

This failure occurs entirely within the regime of adding/subtracting DatePeriod to/from LocalDate. I think this is a caveat/behavior that users simply have to be aware of, like the above quirks of numeric types. Once aware of it, there are use cases where DatePeriod addition may be desired, such as the following:

val ld1 = LocalDate(2000, 1, 31)
val dp1 = DatePeriod(months = 1)

val twoMonthsFromNow = (ld1 + dp1) + dp1
val twoMonthsFromNowPreserveDayOfMonth = ld1 + (dp1 + dp1)

This is what we were discussing in terms of DateRange; which of the two is more intuitive and should be the default for iterating through dates. It may even be desirable to provide an optional parameter that allows advanced users to choose which behavior the progression will utilize.

@dkhalanskyjb
Copy link
Collaborator Author

I don't think the issue is with DateTimePeriod.plus and DatePeriod.plus exactly. Date{Time}Period addition is both commutative and associative

Yes, but this wasn't my point. The point was that the sum of "4 months and 23 days of waiting", and then "3 months and 23 days of waiting" isn't "7 months and 46 days of waiting", so the obvious semantics of + on date periods ("the sum of periods is a period, waiting for which can be split into the constituent periods") isn't provided. What semantics is provided, then, and when is it even useful?

It is somewhat reminiscent of when the limitations of numeric types yield different behavior than pure math, such as the following failing tests:

All of these examples use operations that make sense, they have a well-defined meaning. 1 / 2 is division, rounded toward zero. A clear concept that is often useful.

This failure occurs entirely within the regime of adding/subtracting DatePeriod to/from LocalDate. I think this is a caveat/behavior that users simply have to be aware of, like the above quirks of numeric types.

Agreed here. date.plus(n, DateTimeUnit.MONTH) has a clear purpose: we're looking for a date with the month n months later than this date and the day-of-month the closest possible to the source day-of-month. To support this purpose, yes, we have to deal with irregularities, like with numeric types. It's better than the alternatives that don't guarantee that the month will be the desired one, e.g. 2024-01-31 + 1 month == 2024-03-02.

Whether adding together two periods has a purpose is up for discussion (hence the issue).

Once aware of it, there are use cases where DatePeriod addition may be desired, such as the following:

How about this instead?

val ld1 = LocalDate(2000, 1, 31)
val dp1 = DateTimeUnit.MONTH

val twoMonthsFromNow = dp1.plus(1, dp1).plus(1, dp1)
val twoMonthsFromNowPreserveDayOfMonth = ld1.plus(2, dp1)

I find this much clearer: usually, parens don't matter in expressions like a + (b + c) (or only have a very minor and deeply technical effect, like in floating-point numbers), so one may be tempted to just write this as a + b + c without being aware of accidentally changing the result.

@PeterAttardo
Copy link

The more I go down this rabbit hole, the deeper it gets.

Returning to the definition of a DatePeriod as the difference between two LocalDates, I'm having an issue with that definition. The difference between two LocalDates can be expressed as an infinite number of different DatePeriods (which are not canonically equivalent to each other, despite being defined by the same two dates), and a DatePeriod can correspond to an infinite number of pairs of LocalDates. While the two concepts are certainly related, I'm not sure it can be said that one defines the other. To this point I have been intuitively defining a DatePeriod as the composition of some quantity of DateTimeUnits (specifically months and days).

If we return to greenfield, restricting ourselves solely to periods of the unit Day yields no problems. It is associative and commutative both with itself and with LocalDate. There are no surprises with addition or subtraction. The period between two dates is always a single value. All of the problems we've been discussing go away. However, date periods of months (and years, which are defined in months) are useful, and so we have a desire to accommodate them.

I'm spitballing a little bit here rather than making a hard proposal, but what if we split DatePeriod into unit-specific classes? So there would instead be DayPeriod, MonthPeriod and YearPeriod. DayPeriod would be the default, and would behave as people expect (no caveats on operator addition, etc). LocalDate subtraction would return DayPeriod. If one wanted to replicate the existing subtraction behavior, which yields months and days, they could use division and modulus on a DateRange. So (date1..date2) / DateTimeUnit.MONTH would yield the number of months between the two dates and (date1..date2) % DateTimeUnit.MONTH would yield the number of days remaining after the maximum integer number of months is accounted for. MonthPeriod and YearPeriod would have toYearPeriod() and toMonthPeriod() respectively, but neither would be directly convertible to DayPeriod. There could also be a CompositePeriod that is essentially a List<Period> and computes addition in the order of the list (so the current DatePeriod(months=1, days=6) would become CompositePeriod(MonthPeriod(1), DayPeriod(6)).

The advantage of this approach is that it would allow for a sane default (days-only) in all cases, while still allowing for more complex uses for users who deliberately choose it. If you really wanted to you could even enforce this by using the OptIn annotation on MonthPeriod and YearPeriod.

@dkhalanskyjb
Copy link
Collaborator Author

Returning to the definition of a DatePeriod as the difference between two LocalDates, I'm having an issue with that definition.

Yes, you're right: it's not the difference, it's a difference.

The difference between two LocalDates can be expressed as an infinite number of different DatePeriods

The number of representations becomes finite if we limit ourselves to periods where all components have the same sign. Then, for dates, there are date1.until(date2, DateTimeUnit.MONTH) different periods, one for each month converted to the corresponding number of days.

In practical terms, this shouldn't be a problem: usually, either you want just the number of days (so you call daysUntil), or you do want as much time to be counted as months (so you call periodUntil). Periods like 5 months and 63 days should be extremely rarely relevant. Likewise, people usually don't record lengths like 2 meters and 133 centimeters: it's either 3 meters and 33 centimeters or 333 centimeters.

To this point I have been intuitively defining a DatePeriod as the composition of some quantity of DateTimeUnits (specifically months and days).

That's a good way to think about DatePeriod, yes.

I'm spitballing a little bit here rather than making a hard proposal, but what if we split DatePeriod into unit-specific classes? So there would instead be DayPeriod, MonthPeriod and YearPeriod. [...] There could also be a CompositePeriod that is essentially a List<Period> and computes addition in the order of the list

This is a valid standardized generalization of DateTimePeriod (which ISO 8601 calls a "duration"). In ISO 8601-2:2019(E), there's an example in 11.3.3 Precedence representation that that shows how such durations are defined:

'P2DP3MP1Y' describes a duration of two days, three months and one year, to be evaluated in the order from left to right.

So, yes, it's possible to define composite durations as lists of duration components, and this would make addition (and therefore, multiplication by a scalar) well defined.

Here's a reason not to do that: we don't want to miss the forest for the trees. In the end, our library is supposed to help the programmers, not to force them to research how dates and times work. DateTimePeriod is a fairly predictable thing that should typically do what you expect: if you see a countdown like 2 months, 3 days, and 6416 seconds on a website, the arithmetic operations you'd do in your head would probably be exactly the ones implemented by DateTimePeriod. Silently doing the right thing is a good quality that we don't want to give up.

Technically, the CompositePeriod is a more flexible and general entity, but no one has asked us for such flexibility yet, and if we were to introduce CompositePeriod, everyone would have to shoulder this complexity for no clear gain: now, to implement the ubiquituous 2 months, 3 days, and 6416 seconds logic, you can't just take DateTimePeriod, you have to learn about the non-commutative addition of periods to datetimes, choose the precedence, etc., only to arrive to a thing that's now provided out of the box.

We can still introduce CompositePeriod if there's actual demand for it, but probably not at the expense of the more streamlined DateTimePeriod.

The advantage of this approach is that it would allow for a sane default (days-only) in all cases

If I'm looking at the difference between 2024-04-23 and 2025-04-23, I'd say that the sane thing to say is that a year has passed between them. The default you're proposing is "sane" in the sense that it's computationally simple and easy to reason about, but, in my view, not in the sense of what one could realistically want from the operation that returns a difference between two dates.

If one wanted to replicate the existing subtraction behavior, which yields months and days, they could use division and modulus on a DateRange. So (date1..date2) / DateTimeUnit.MONTH would yield the number of months between the two dates and (date1..date2) % DateTimeUnit.MONTH would yield the number of days remaining after the maximum integer number of months is accounted for.

These building blocks are all already present: we have date1.until(date2, DateTimeUnti.MONTH), and periodUntil is defined in terms of until. The difference from your proposal is that we return just an Int or a Long and not the Period values. We could add types here, but we'd need a reason to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This could break existing code
Projects
None yet
Development

No branches or pull requests

2 participants