Morph elapsed_years() API into years_since() #707
Conversation
There was a problem hiding this comment.
tbh I think it's complementary.
elapsed_years()reminds me ofstd::time::Instant::elapsed()https://doc.rust-lang.org/std/time/struct.Instant.html#method.elapsed its API is more concise and fit a particular use case: how many years has been spent since... birthday, subscription date, posted, etc...years_since()is probably useful. I don't have a use case in mind though.
It doesn't cost much to implement both because elapsed_years can call years_since.
| /// Constants that can be used by all components. | ||
| pub mod consts { | ||
| /// Constants of type `f32`. | ||
| pub mod f32 { | ||
| /// Number of weeks in a year. | ||
| pub const WEEKS_PER_YEAR: f32 = 52.1775; | ||
| } | ||
| /// Constants of type `f64`. | ||
| pub mod f64 { | ||
| /// Number of weeks in a year. | ||
| pub const WEEKS_PER_YEAR: f64 = 52.1775; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not "imprecise". This actually very precise: https://en.wikipedia.org/wiki/Week#Definition_and_duration
There was a problem hiding this comment.
Well, if you define a YEAR to be a "mean year" computed across 400 years, then yes. For the conventional meaning of a year (of either 365 or 366 days) it isn't very precise.
There was a problem hiding this comment.
So I guess it's just the naming that is incorrect
There was a problem hiding this comment.
I'm not sure to understand, 365.2425 days is more precise than either 365 or 366 days.
There was a problem hiding this comment.
All other things being equal I'd prefer not to expose relatively niche API surface.
There was a problem hiding this comment.
It's not like the number of weeks in a mean year is going to change though
src/naive/datetime/tests.rs
Outdated
| let one_year_ago = Utc::today() - Duration::weeks((WEEKS_PER_YEAR * 1.5).ceil() as i64); | ||
| // A bit more than 2 years. | ||
| let two_year_ago = Utc::today() - Duration::weeks((WEEKS_PER_YEAR * 2.5).ceil() as i64); |
There was a problem hiding this comment.
You should probably not use today here but give a specific date. This test is tricky to get it right. @yozhgoor did it right but it isn't easy to understand why.
If you give a specific date, it will be right all the time and it's easier to understand.
There was a problem hiding this comment.
I mean: the test success depends on the exact date you are running the test. If you get it wrong you can easily get a flaky test that breaks sometimes every 4 years or so.
There was a problem hiding this comment.
The tests I had here (which I had moved around due to some confusion) are direct ports of the original tests to the new API), so I don't think this is different from before?
There was a problem hiding this comment.
I mean the test is right and will succeed but it depends on an external variable that is the current date and it is hard to read and understand why it will always succeed.
It's safer and easier to use a specific date (take the current date for instance) so it doesn't vary each time the test is ran.
So my point is not about the test being wrong or right but the maintainability.
There was a problem hiding this comment.
Ah sorry, I thought you were implying I had regressed this test compared to the previous version.
I'm honestly not sure if I agree that using a specific date would be safer and easier. Technically, yes, some non-deterministic behavior is introduced by relying on the current date, but given the constants used I feel these tests are pretty robust and the maintainability doesn't suffer enough for me to go and rewrite them.
|
I agree to some extent that they're complementary. At the same time, I'd argue that both are relatively niche (if only for the fact that we've managed to get by without them for many years) and, given that I'm here as a replacement maintainer trying to get a handle on maintenance I'd prefer to have less public API, so I'm inclined to stick with only the more powerful API for now. (TBH if someone were to submit a PR adding back the other one with some test coverage, I'd probably merge it.) |
Since @yozhgoor originally made the API for elapsed_years instead of years_since despite it would have been easier, I expect that he would probably benefit from it. |
|
Thanks for the feedback! |
@yozhgoor would you like to review? This:
Noneif thebaseis in the future[cfg(feature = "clock")]since it doesn't rely onUtc::today()