-
Notifications
You must be signed in to change notification settings - Fork 517
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Months::num_months() and num_years() #1373
Add Months::num_months() and num_years() #1373
Conversation
496dc9c
to
9d44606
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
How are you using |
@djc I can understand that, but I'm not against 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 I hope that helps...? |
I don't think you've really answered the question of how/why you're using it? |
@djc apologies, I thought I had. In code using Chrono at present, In some places we have code that emits In order to perform appropriate testing, it is necessary to confirm and validate what value of 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 🙂 |
Okay, I'll take an |
@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 🙂 |
Thanks! |
The additions in this commit are dependent upon as-yet unreleased Chrono functionality, which will arrive in Chrono 0.4.32. Specifically, this PR has been merged, allowing inspection of the internal quantity of Months: chronotope/chrono#1373 Due to the Chrono maintainers preferring Months.as_u32(), this commit adds Duration-style convenience methods as an external extension to the chrono::Months type. - Added num_months() and num_years() to extend chrono::Months. - Added tests and documentation.
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 toDuration
.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 insrc/offset/local/tz_info/rule.rs
, but nothing usable. Please advise if one should be added instead of using the magic number12
- 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 asinline
, and some asmust_use
, but not entirely consistently. In an attempt to fit with the codebase as well as possible, the two new functions have been annotated asinline
, but not asmust_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.