Month::overflowing_add/sub

[esheppa]

Add an overflowing_add/sub similar to the equivalents for NaiveTime, which are useful for NaiveDate + Months
Also change Months constructor to only accept u32 <= i32::MAX and add a try_new

[esheppa]

If this is looking like a good API, I'll add some more documentation prior to this being merged

[esheppa]

I still need to do the MIN_YEAR / MAX_YEAR checks in diff_months as well

[djc]

In principle I'm open to the modularization of moving some of diff_months()'s internals into public API on Month, but I'm not sure this API is really natural/otherwise useful.

[djc]

No more overflow/underflow checking for the years?

[esheppa]

Accidentally removed too much from the function. I've added it back as:

        if year < MIN_YEAR || year > MAX_YEAR {
            return None;
        }

Or do you mean the self.year() + years overflowing/underflowing the i32?

[djc]

Both...

[esheppa]

will add the check on i32 as well

[djc]

I don't see the point of using num_traits::FromPrimitive and using unwrap() if we can just use as here.

[esheppa]

I need the actual Month type here as that's what overflowing_add/sub take as the &self parameter. This will be able to be cleaned up later if/when we fix #727

[djc]

FromPrimitive is just too opaque to me, IMO something lower-level is easier to reason about.

[esheppa]

I don't think it is possible to go from u32 to an enumeration thought?

[djc]

I'm not really a fan of this as the constraint ends up not being expressed in the type system, so we have to lean on convention/remembering/understanding. IMO it's better just to it as u32 (which also makes the initialization API simple) and check the range in usage context.

[esheppa]

My thinking with this is that if the Months is invalid, its better for this to show up when creating the months rather than adding it, but admittedly most Months will be made and added as part of the same expression. Perhaps another option is taking i32 here but returning None on negative values? Or else removing new_opt and having the check be done inside overflowing_add/sub. This also forces months to have one less possible value on the sub side due to i32::MAX != abs(i32::MIN), but potentially this is acceptable as it is erring on the side of allowing a slightly smaller maximum subtraction?

[djc]

But that's just the point: a Months::new(u32::MAX) is not invalid by itself -- it's conceptually perfectly valid. It's only invalid in the context of what checked_add_months()/checked_sub_months() are trying to do, as such IMO the burden of validation should fall on the context, not the type itself.

[esheppa]

Makes sense, will move the checks there

[djc]

What does the test_ prefix buy us? This seems like an unrelated change.

[djc]

I'd prefer to keep the empty line here

[esheppa]

With regard to the API, I agree that it is debatable if it is generally useful. A couple of potential use cases are:

• Users wanting to implement add_months but with their own clamping behavior on invalid days
• Users wanting to know the month n months after another, irrespective of the year

Alternatively if you like this but don't think think the public API is useful, it could be made private?

[djc]

I think this is only good if the public API is useful. Otherwise the indirection makes the code harder to understand and I'd prefer to keep it as I had it in my original PR. It sounds to me like we don't yet have a very convincing use case in mind for exposing this publicly, so in that case I think we should leave this for now and keep this in mind as a potential direction for future evolution.

[djc]

(Sorry, closed it accidentally.)