Create add_months() method on NaiveDate

[avantgardnerio]

The Apache arrow-datafusion project is a query engine that supports doing date math with arrow primitives like IntervalYearMonth, which despite being a common part of SQL standards, adding months to a NaiveDate was surprisingly non-trivial to perform correctly.

Support is being added to arrow-datafusion in this PR, but it seemed like logic like this might best make it's home in the chrono crate, so that others don't have to re-implement this logic. A quick search of issues reveals some demand:

• plus_months function of the DateLike trait #474
• Datelike should have methods to add/sub a number of calendar day/month/year #290

Unfortunately, I realize there is no Duration::Month because it doesn't represent a fixed duration in time, so hopefully this is an acceptable compromise. If not, I defer to the maintainers on the best way to implement such functionality.

[djc]

@esheppa I believe you referenced some external crates that add stuff like this? I think I tend to agree that it probably make sense to bring this functionality into chrono, what do you think?

[avantgardnerio]

warning: unused import: `micromath::F32Ext`
  --> src/naive/date.rs:11:5
   |
11 | use micromath::F32Ext;
   |     ^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: `chrono` (lib) generated 1 warning

I don't usually do no_std - does anyone more familiar have a suggestion on how to resolve this error only present when compiling with std?

[avantgardnerio]

The no_std issue has been solved, thanks!

[esheppa]

I think it would be great to have an add_months function in chrono like this, as it is a commonly requested feature, the behavior matches the generally expected way that this kind of thing works, and the implementation is complicated enough that it's not ideal to have it as an example. From my experience this functionality is also useful in many business use cases so will likely be useful to many users of chrono outside of arrow-datafusion.

One of the crates I mentioned is chronoutil which has shift_months which behaves similarly to add_months here.

The other way this kind of thing can be achieved is by using a YearMonth type, which can have months added or subtracted, and can be converted into a date, eg via start_date, end_date, with_day and with_closest_day functions. This is nice as it is entirely unambiguous how adding months to a given YearMonth will work, and it seems cleaner to store only the minimum amount of information required (same as preferring a Date over a DateTime when the time part isn't relevant to the use case). YearMonth is mentioned in past chrono issues and PR's: #69 (comment), #152, #184

TLDR:
I think it is definitely worth having the add_months functionality. We could also consider having a YearMonth data type, but that may better be in an external library as it likely won't depend on any internals of chrono (and there are many other useful contiguous time resolutions other than YearMonth).

[avantgardnerio]

Thanks! I swapped out to shift_months and all our tests still pass, so I think our needs are satisfied. It would be good if this was moved into chrono, just so we didn't have to pull in yet another crate, but I'll close this PR for now.

[esheppa]

Hi @avantgardnerio - I'm still keen to merge this if @djc is also on board. There may be some changes required by code review but I think this functionality would be great to have within chrono. That said v0.4.20 is not yet released, which is when this feature would be available, so using chronoutil in the short term will solve your immediate need, then if you like you would be able to remove that dependency once chrono v0.4.20 is available.

[avantgardnerio]

@esheppa , I'd like to push back against the idea of a YearMonth being related to an instant in time, as proposed in #69 and #152, or at least that being outside the scope of this PR. Just glancing at it, the solution proposed in #184 is a lot more in line with my expectations: YearMonth being a calendar duration, not an instant. I think instant-based YearMonth operations are less common, and Chrono already supports them in methods such as NaiveDate::from_ymd().

If we wanted something more more type-safe than the i32-based solution, I'd propose something like JodaTime's Period. I do think this approach is overly complicated however, since all other durations are true fixed intervals: hours, minutes, seconds, days, and weeks are a defined as number of relative milliseconds (ignoring leap seconds, which even Google does).

Unfortunately the outliers are months and years (which are trivial for users to treat as 12 months). With this being the single edge case, I'd posit that adding a new enum is probably not be worth the complexity: would we need a new NaiveDate.add_calendar_duration() method?

Looking for precedent, it appears both JodaTime and Java 8 support the straightforward solution like the one proposed in this PR:

• https://stackoverflow.com/questions/25300140/add-month-with-jodatime
• https://docs.oracle.com/javase/8/docs/api/java/time/LocalDate.html#plusMonths-long-

I'm happy to help do the work to finally get month addition into chrono, but it seems like the first step might be selecting a commonly agreed upon design.

[esheppa]

Thanks for adding this method. The code looks good - if possible I'd just like a few changes like removing use of as (I recognize this is used heavily around the code base but we are trying to avoid introducing new usage, and gradually removing the old usage)

[esheppa]

Hi @avantgardnerio - thanks for these notes, they raise some valuable points in the YearMonth and equivalents discussion - if you don't mind, may I re-post them at #722 which I'll try to use as a coordinating issue for a discussion around if/how to handle YearMonth and other related things like first and last days of specific periods. That will also allow us to focus specifically on the add_months function here (my apologies for expanding the scope initially).

In terms of having an i32 or Period style interface for the add_months, I'm quite comfortable with sticking with an i32 interface as I think the JodaTime style Period is more closely related to the other items to be discussed in #722.

[avantgardnerio]

It looks like the build is failing because i32::try_from() exists in the std library... I would appreciate advice on how to resolve that, thanks!

[esheppa]

Thanks for the changes, they look great! Unfortunately it looks like try_from is only available in 1.34 (in either std or core, we just need to change the imprort to get the trait from core), apologies as I should have checked that first - let me check with @djc if a move to 1.34 MSRV is possible, otherwise looks like we have to either use one of the following options:

1. Go back to using as (IMO not preferable)
2. Wait for version 0.5
3. Put this behind a feature flag that could in future include other code that doesn't meet MSRV (my favored option as this allows us to add other code using this technique as well, and it seems reasonable that users on the MSRV might miss out on a few features)

[djc]

If this is not fallible I'm inclined not to have this interface in favor of a Months type that we can use with std::ops::Add.

[avantgardnerio]

If this is not fallible

I don't entirely understand... does "fallible" have a domain-specific definition here?

[esheppa]

@djc - The reason I'm relatively keen on this API is that I think it will take a while to figure out how a potentially more ergonomic Months style API could look and even when that is figured out this could be useful in implementing it. Essentially this PR allows us to punt the discussion of Month and other data types to #722.

Potentially this being behind an unstable feature flag and disabled by default would allow us to get this out in 0.4.20 and then either keep it or provide a potentially more ergonomic API in 0.5 (the unstable feature flag would also - potentially - enable using TryFrom instead of as, provided we document that the unstable feature has a higher MSRV) (hopefully I've got the application of unstable correct here compared with #711 (comment) however I recognize that this doesn't add any new dependencies, and potentially makes API discovery trickier if a user wants add_months and then realizes that they have to add a feature to get it)

Alternatively, as this doesn't rely on any private APIs, this could be moved to documentation or an example as an interim solution.

[djc]

I guess my argument is that we could just do a minimal struct Months(usize); with maybe constructors for months(num: usize), years(num: usize) and a single impl Add<Months> for NaiveDate. As far as I can tell for this we can just move the code we already have in this PR around to move to a type-based approach, which wouldn't preclude us from adding more functionality to Months later, generalizing the API.

My argument against add_months() is that it's just another one-off method with a decent chance of getting deprecated down the line as we figure out the design constraints for #722, while there's a actually a more-idiomatic type-based approach that can do the same thing.

[avantgardnerio]

do a minimal struct Months(usize);

I'd be happy to make this change if it's generally agreed upon. It seems like a good compromise.

[djc]

You got a thumbs up from @esheppa, I think that means we have consensus? (Just calling it out to generate a notification.)

[esheppa]

Sorry, started typing this earlier and got distracted. I think it makes sense to go ahead with a Months type and Add and Sub impls, with the constructors @djc mentioned.

On a seperate note - @djc - what is your preference with regards to preferring TryFrom instead of as? If we lean toward TryFrom then this functionality would need to be feature gated as the relevant TryFrom impls are only available in rust 1.34

[djc]

I don't think TryFrom is worth the added complexity of MSRV shenanigans for now. Let's reconsider bumping the MSRV to something a little more sane (maybe 1.48) after the 0.4.20 dust settles. However, it would be good to make any potential panicking explicit in the code rather than as an under/overflow.

[esheppa]

My apologies @avantgardnerio as I know I'd requested earlier to switch usage of as to TryFrom, but it looks like we will aim to do this in future releases rather than 0.4.20, so if possible can you switch it back to how it was before with the as usage?

[avantgardnerio]

can you switch it back

No worries. I wasn't sure where to put the Months struct, but I think the rest of the PR conforms with the prior feedback.

[esheppa]

Looks great, thanks for your patience @avantgardnerio - will be great to have this in 0.4.20!

[esheppa]

Thanks @avantgardnerio

[avantgardnerio]

Thanks @avantgardnerio

Thanks you :) Very happy to have this in chrono.