Added length function to Month so that I don't have to use time crate #1643
Conversation
DanielPoprawski
temporarily deployed
to
Coverage
GitHub Actions
Inactive
src/month.rs
Outdated
| | Month::October | ||
| | Month::December => 31, | ||
| Month::April | Month::June | Month::September | Month::November => 30, | ||
| Month::February if year % 4 == 0 && (year % 25 != 0 || year % 16 == 0) => 29, |
There was a problem hiding this comment.
The logic for February isn't fully accurate, and I think we have API for this on Date that you could repurpose.
There was a problem hiding this comment.
I found the functions in question under NaiveDate and then under internals.rs but it seems to be meant for testing purposes only and it's unreachable from the month.rs file
There was a problem hiding this comment.
Feel free to expose whatever this needs as pub(crate), I don't want to duplicate this logic here.
src/month.rs
Outdated
| @@ -161,6 +161,22 @@ impl Month { | |||
| Month::December => "December", | |||
| } | |||
| } | |||
|
|
|||
| /// Get the length of the month in a given year | |||
| pub const fn length(&self, year: i32) -> u8 { | |||
There was a problem hiding this comment.
Not sure about this name. len() would be more idiomatic in Rust but maybe it should be a little more specific like num_days()?
There was a problem hiding this comment.
I think length is a pretty straightforward explanation of what the function does. I didn't like how num_days() sounds
There was a problem hiding this comment.
I disagree -- len()/length() is not very specific and not fully intuitive on Month IMO -- for example, it could yield a Duration instead. We already use the num_days() for a number of methods in chrono and it would be a good fit here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1643 +/- ##
==========================================
- Coverage 91.11% 90.88% -0.24%
==========================================
Files 37 37
Lines 17137 17128 -9
==========================================
- Hits 15614 15566 -48
- Misses 1523 1562 +39 ☔ View full report in Codecov by Sentry. |
…s seems to be for testing purposes so I left it alone but updated the equation. Every 4th year is a leap year, except for if it is the first year of a century, except-except every 400 years
DanielPoprawski
temporarily deployed
to
Coverage
GitHub Actions
Inactive
|
I'll do it myself. |
Thanks for contributing to chrono!
If your feature is semver-compatible, please target the main branch;
for semver-incompatible changes, please target the
0.5.xbranch.Please consider adding a test to ensure your bug fix/feature will not break in the future.