parse_datetime fix relative month dates result in wrong dates #253
Conversation
|
could you please add tests to cover this ? |
|
Are these tests sufficient? @sylvestre |
| // *NOTE* This is done in this way to conform to GNU behavior. | ||
| let days = dt.date().last_of_month().day() as i32; | ||
| Span::new().try_days(days.checked_mul(x).ok_or("multiplication overflow")?)? | ||
| let mut temp = dt.clone(); |
There was a problem hiding this comment.
please rename the variable for something more usefull
|
|
||
| #[rstest] | ||
| #[case::months_ago_1("2026-01-12 1 Months ago", "2025-12-12")] | ||
| #[case::months_ago_2("2026-01-12 2 Months ago", "2025-11-12")] |
There was a problem hiding this comment.
we don't need all the months, just a few will be ok
There was a problem hiding this comment.
consider adding edge cases like 0, 24, 36 months
and very big values and negative
There was a problem hiding this comment.
and missing tests for leap year scenarios and end-of-month edge cases (e.g., Jan 31 + 1 month)
There was a problem hiding this comment.
specifically end of month edge cases.
How are these wished to be handled?
Jan 31 + 1 Month = 3rd March or 28/29. Feb (depending on leap year?)
There was a problem hiding this comment.
If we say 31.1 + 1 Month = 28/29 there is actually a checked_add with just a Span of 1 Month that has this exact behaviour. This would simplify the whole codebase. But the specific testcase of an overflow for 31 Jan + 1 Month = 3rd March exists.
|
|
||
| #[rstest] | ||
| #[case::months_in_1_months("2026-01-12 1 Months", "2026-02-12")] | ||
| #[case::months_in_2_months("2026-01-12 2 Months", "2026-03-12")] |
| let mut temp = dt.clone(); | ||
| let mut days: i32 = 0; | ||
| if x < 0 { | ||
| for _ in 0..x.unsigned_abs() as usize { |
There was a problem hiding this comment.
Using unsigned_abs() as usize could panic on very large values
please add tests to cover this too
|
@sylvestre This would result in 12.Jan - 2 months -> 11 Sep. In the testsuite for this repo there is 1 testcase which explicitly tests that such overflows/underflows happen and are not handled correctly (correctly is subjective in this case i guess?). Eg 31.Jan + 1 Month. If we say it should overflow because thats what GNU does this whole issue and the source issue (uutils/coreutils#10185) can be closed with the argument of thats the GNU way of not guarding overflows/underflows. Just want a guiding opinion before i remove existing testcases explicitly for these under/overflows. For me logically i would try to make it so that the day of the month always stays the same, unless the target month has less day (eg 31 jan -> 29 feb). |
|
i think we want to do just like GNU :) |
fixed: #252
Using "X months ago" or "X months" (into the future) resulted in wrong dates if the months had different amount of days. Previously parse_datetime took the days of the current month and multiplied it times x to get the day delta between now and the desired X months ago. If this month had 31 days but the previous month had only 30 days this would result in -1 month and -1 day.
New implementation loops x times and checks the days in the months to calculate the correct day delta.