Use chrono::{Months,Days} for correct calculation#28
Use chrono::{Months,Days} for correct calculation#28simon04 wants to merge 4 commits intouutils:mainfrom
Conversation
The duration (amount of seconds) of a year, month, week, days depends on the reference date. Introduces `add_relative_str` to add the relative duration to the given DateTime instance.
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
Too bad chrono doesn't have some more general Duration type for this. Once this gets more complex we might need to make one ourselves. This looks like a pretty good workaround in the meantime.
Also, @sylvestre Looks like I don't have the rights to approve the CI here :)
|
@simon04 sorry for the latency, i was on holidays @tertsdiepraam permissions updated |
tertsdiepraam
left a comment
tertsdiepraam
left a comment
There was a problem hiding this comment.
Alright, let's get back to this. I think this is an important and good fix, so I want to get it merged, but we need to take some other changes into account.
src/lib.rs
Outdated
| pub fn from_str_at_date(date: NaiveDate, s: &str) -> Result<Duration, ParseDurationError> { | ||
| let time_now = Local::now() | ||
| .date_naive() | ||
| .and_hms_opt(0, 0, 0) | ||
| .unwrap() | ||
| .and_utc(); |
There was a problem hiding this comment.
I think the most sensible thing to do is to say that this function should always accept some reference DateTime<Tz> instead of a naive date. That's because this function needs to be timezone-aware as well. Using Local specifically makes this harder to test and leads to issues like #36. Maybe that makes splitting off add_relative_str into a separate function unnecessary?
This also ties in with #33, because we should be focusing on returning DateTimes, not Durations. So maybe the add_relative_str is actually all we need? What do you think?
There was a problem hiding this comment.
I kept from_str_at_date for backwards compatibility. How do you deal with breaking changes in this library?
I agree, any function returning a Duration should be deprecated.
There was a problem hiding this comment.
Since the library is very new, I think we should make all these changes sooner rather than later. Once the library settles a bit more we can think about stronger stability guarantees.
There was a problem hiding this comment.
Alright, I'll remove from_str and from_str_at_date, and adapt the unit tests to call add_relative_str instead...
The duration (amount of seconds) of a year, month, week, days depends on the reference date and timezone. Thus we only provide the unambiguous `add_relative_str`.
|
sorry but could you please rebase it ? :( |
3 participants
The duration (amount of seconds) of a year, month, week, days depends on the reference date.
Introduces
add_relative_strto add the relative duration to the given DateTime instance.Fixes uutils/coreutils#4811.