Add Months::num_months() and num_years()

[danwilliams]

Summary of changes

The changes in this PR are extremely simple and trivial: to present two new functions that provide general ease and compatibility in using Months, in similar fashion to Duration.

The problem being solved is that the Months type is a one-way black box. Once created, it is impossible to verify what it contains. Although its primary purpose is to provide a vector for effecting changes, without being able to inspect its value it is impossible to verify in tests, or support in a wider context. Additionally, extending the type from an external perspective will not help with this.

Therefore, this PR simply adds two new methods to Months:

• Months::num_months() - This returns the number of months represented by the type, and is the primary objective of the PR.
• Months::num_years() - This is a convenience function that is added to fit with the functions provided elsewhere.

The naming of the functions follows those provided by Duration, as this is the most similar type.

Questions and points of note

Constants

There is no readily-available MONTHS_PER_YEAR or similar - there is one in src/offset/local/tz_info/rule.rs, but nothing usable. Please advise if one should be added instead of using the magic number 12 - in which case, should it be local to the module, and private, or would Chrono benefit from a more public one?

Annotations

Observing the code elsewhere, for instance in Duration, some functions are annotated as inline, and some as must_use, but not entirely consistently. In an attempt to fit with the codebase as well as possible, the two new functions have been annotated as inline, but not as must_use. Please advise as to what the general Chrono preference is in this regard.

Tests

Tests have been added to try to satisfy what seems sensible and appropriate to check, in a manner that fits in with the other Chrono tests. Hopefully they have been placed in an acceptable location.

Branch point

The branch point chosen has been the latest tagged release, with the PR targeting the 0.4.x branch. Hopefully that is correct.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e730c6a) 91.35% compared to head (b56e3a4) 91.68%.
Report is 72 commits behind head on 0.4.x.

Additional details and impacted files

@@            Coverage Diff             @@
##            0.4.x    #1373      +/-   ##
==========================================
+ Coverage   91.35%   91.68%   +0.32%     
==========================================
  Files          38       38              
  Lines       17034    17586     +552     
==========================================
+ Hits        15562    16123     +561     
+ Misses       1472     1463       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[djc]

How are you using Months that you need this? I somewhat regret adding Months to the API and it's not obvious to me that it should be used in downstream code to the effect that these sorts of methods are needed.

[danwilliams]

@djc I can understand that, but I'm not against Months existing, as it serves a purpose 🙂

The trouble is, as soon as you start using it (and it is the natural way to carry out calendar-based changes), you then need to be able to test and verify the code that's using it, and that starts to rely upon it. So anywhere that Months is used, needs to be able to verify the quantity that the instance represents. Otherwise, the type is not really usable.

I hope that helps...?

[djc]

I don't think you've really answered the question of how/why you're using it?

[danwilliams]

@djc apologies, I thought I had.

In code using Chrono at present, Months is being used where appropriate.

In some places we have code that emits Months as part of the general interface dealing with dates, times, durations, etc. - all based on Chrono.

In order to perform appropriate testing, it is necessary to confirm and validate what value of Months is being given to the instances in use.

This applies more broadly, as any type used for any purpose needs to be able to be inspected to the degree where its correctness is verifiable. Hence I had thought my previous explanation would help there. However, I trust this clarification has helped resolve my earlier oversight 🙂

[djc]

Okay, I'll take an as_u32() method (and would prefer to avoid the added convenience method for now). The way forward here will be #1290.

[danwilliams]

@djc no problem! I will amend and resubmit the PR 👍

I took a quick look at #1290, and that looks interesting. Are you open to discussion on it? I have a complete and working implementation of Intervals, which include calendar-based durations, which I was considering adapting for potential submission to Chrono at some future point. I did not know work had already been done on something similar. From doing that I can see some drawbacks in the current implementation in #1290, which it might be worth discussing? Perhaps that's why development on the feature has stalled? If it's all in hand and no help is wanted, I will back politely away 🙂

[djc]

Happy to take your feedback in #1290 (not #1291)!

[djc]

Thanks!