Consider using different representation for CalendarDuration

[quodlibetor]

Per the request in chronotope/chrono#184 I'm going to go ahead and open some code-review comments in here.

The first thing that a deeper reading of the code gives me is the shape of the CalendarDuration struct. It combines a time::Duration and some calendar-centric items like months and years.

What does it mean to add a duration that is 1 month and 5 seconds? How do we do the bookkeeping for leap hours and leap seconds?

Looking at other languages that have similar constructs:

• java.time.Period has just Year, Month, Day
• c++ boost.date_time.gregorian is (seems to be) the same. The STL appears to only have a timespan-based duration. I have heard that there are plans for a revamped C++ datetime library, but I haven't ready much about it.
• Python is similar and only has timedelta, which is not the same as what we're trying here. There is, instead, the python-dateutil package's relativedelta class, which is kind of a combination of what you've got here and the interfaces in the next section.

Then we get into languages that have methods directly on DateTime types. I'm just going to talk about add_months, since it has the same problems as add_years and lets us ignore the difference between add_seconds(86_400) and add_days(1).

• C# (and the rest of .NET) has just AddMonths on the DateTime type.
• Haskell... honestly I'm not sure what's going on with haskell. From this tutorial it seems like it's got the same basic design as C#.

So the takeaway from this is: most languages only provide seconds-based durations in the std (like rust) but when you start allowing calendar manipulation a clear distinction between temporal durations and calendar periods is important.

To that end, I think I'd rather that the shape of the CalendarDuration struct looked like:

struct CalendarDuration {
    Days: i32,
    Months: i32,
    Years: i32,
}

With corresponding constructors. If users want something like Python's relativedelta API, then it's much more clear to do something like:

fn add_business_delay(my_dt: DateTime) -> DateTime {
    my_dt
        + CalendarDuration::from_months(5)
        + Duration::from_millis(5000)
}

Than something like:

fn add_business_delay(my_dt: DateTime) -> DateTime {
    my_dt
        + CalendarDuration::months(5).seconds(5)
}

When you start iterating over things like that, being able to separate out parts of the logic that depend on timezones and Leaps from parts that are purely calendar centric seems, to me, to be better.

What do you think?

[kosta]

I think your proposal for CalendarDuration makes much more sense. I still think we need a "composite" Duration type, s.th. you can use something like "2 months and 5 seconds" e.g. in an iterator (or have it as data, not as a function).

I was recently made aware of https://github.com/matthiasbeyer/kairos which uses chaining to combine different durations. This way you can say "first add one month and then 5 seconds" or "first add 5 seconds and then two months" which resolves the "arbitraryness" of my approach.

I will try this out in this chrono fork here https://github.com/kosta/chrono/tree/date-iterator but I'm really short on time at the moment (was sick and need to finish something for work)

[quodlibetor]

Sounds great!

[kosta]

Note however that this still has the issue of whether to first add years, months or dates. Maybe CalendarDuration should instead be

enum CalendarDuration {
    Year(i32),
    Month(i32),
    Day(i32),
}

So you can only add years plus months by ordering them via proper chaining.

Also, one might want to specify behaviour or invalid dates, i.e. reject (return None), previous (return Feb 28th instead of Feb 31st) or next (return March 1st instead of Feb 31st), so this would become

enum InvalidDateHandling {
    Reject,
    Previous,
    Next,
}

enum CalendarDuration {
    Year(i32, InvalidDateHandling),
    Month(i32, InvalidDateHandling),
    Day(i32),
}

Does this make enough sense to try to implement it?