- Notifications
You must be signed in to change notification settings - Fork26
Description
I am creating this issue to start a discussion about how to overcome some roadblocks I discovered while trying to address#3505 over atcoreutils
.
Issue 1
To get right to it, the implementation currently used by this library is not consistent with GNU's.
Currently that way thatparse_datetime
parses relative strings is that in runs the string through a regex and createsDuration
objects and adds them together. The inconstancy arises when adding months:it simply adds 30 days; GNU does not do this. Under the existing (parse_datetime
) API2022-05-15 +1 month
will land you on2022-06-14
(After you do somerefactoring to get adate
back not aDuration
) while running GNU'sdate -d'2022-05-15 +1 month
lands you on2022-06-15
. Now, we could have our API increment months usingMonths::new
which returns aDuration
object but that would change the behavior of the existing API and cause a bunch of tests to fail. Why can't we change the tests? Let me explain.
Issue 2
Chrono's API is inconsistent with GNU
While working to solve#3505 I ran into a subtle bug. Addition and subtraction ofMonths
in Chrono's API isnon-commutative. The TL;DR is thatchrono
doesclamping when adding months to their Date APIs. To who understand why this is an issue consider:
#[test] fn test_test(){ let date0 = NaiveDate::from_ymd(2023, 1, 31).add(Months::new(1)).add(Months::new(1)); let date1 = NaiveDate::from_ymd(2023, 1, 31).add(Months::new(2)); assert_eq!(date1, date0); }
Output:
assertion `left == right` failed left: 2023-03-31 right: 2023-03-28
While nothing is being commuted here, it shows that addition of months cannot be commutative under an implementation with this behavior. GNU does not do this. Consider:
[dylan@fedora parse_datetime]$ date -d'2023-01-31 + 1 month + 1 month'Fri Mar 31 12:00:00 AM EDT 2023[dylan@fedora parse_datetime]$ date -d'2023-01-31 + 2 months'Fri Mar 31 12:00:00 AM EDT 2023
While GNU is consistent, Chrono is not. In fact it is 3 whole days off (If "off" is even the right word. What is "correct" is highly debatable). The addition of months is complicated because what it means to add months is somewhat arbitrary. Even how GNU adds months seems inconsistent sometimes. Adding 30 days vs. 1 month, 60 days vs. 2 months results in different dates that ostensibly don't have any obvious connective logic. This behavior further serves to frustrate the issue under discussion!
Whats the Point?
I appears that with these issues, bringingcoreutils
'stouch
anddate
APIs consistent with GNU's under any arbitrary string will be extremely difficult due to how inherently differentchrono
and GNU handle the addition and subtraction of months (maybe even other units of time).
Options
As touched on earlier, we could tweakparse_relative_time.rs
to usechrono
'sMonths::new
instead ofDurations::days
and accept that it will cause a slight change in behavior and just rewrite the tests (What I was tempted to ask for permission to do).
The other option I considered was having a separate implementation for when you want adate
back vs. aDuration
but that seems silly. Users would rightfully assume that the separate functions are two sides of the same coin and would be consistent with each other.
The core issue of this discussion is that either option the result would be behavior that is inconsistent with GNU is some cases. It doesn't appear possible to get away from it.Given any arbitrary string what is the correct order of operations to get the correct date (as defined by GNU) and is it possible to do it withchrono
under their existing implementation???
For what it is worth myintitial pull request to address #3505 did appear to have the requisite functionality but it is unclear if that is the case in forany arbitrary string. And using that would, again, result in a API behavior change.
These issues don't necessarily have obvious solutions which is why I wanted to bring it to theuutils
community.