kosta/date-iterator

date-iterator's Issues

Consider using different representation for `CalendarDuration`

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?

Consider an API more in line with the Rust API guidelines

Your API imitates Chrono's (which makes sense) with constructors like ::days(), but it's much more common in rust to have names like ::from_days() for static constructors.

Chrono is pretty old, and contains some styles that are unexpected to modern Rust programmers. I hope to migrate chrono to something that will cause less confusion for our users.