Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Operations on NaiveDate are not commutative. #1345

Closed
dcechano opened this issue Oct 29, 2023 · 10 comments
Closed

Operations on NaiveDate are not commutative. #1345

dcechano opened this issue Oct 29, 2023 · 10 comments

Comments

@dcechano
Copy link
Contributor

dcechano commented Oct 29, 2023

When taking an arbitrary date, 2022-05-15 for example, and performing operations on that date, the order of operations does appear to matter.

For example, taking the date above and subtracting 20 years, then adding 111 months, then 1000 days is not the same as taking the same date, adding 1000 days, subtracting 20 years and adding 111 months. I have pasted some code below that shows this:

        let expected = NaiveDate::parse_from_str("2014-05-11","%Y-%m-%d").unwrap();

        let mut date = NaiveDate::parse_from_str("2022-05-15", "%Y-%m-%d").unwrap();
        date = date.sub(Months::new(20 * 12)).add(Months::new(111)).add(Duration::days(1000));
        assert_eq!(expected, date);


        let mut date = NaiveDate::parse_from_str("2022-05-15", "%Y-%m-%d").unwrap();
        date = date.add(Duration::days(1000)).sub(Months::new(20 * 12)).add(Months::new(111));
        assert_eq!(expected, date);

The 2nd assertion fails with the date variable showing 2014-05-08, which is 3 days off from the expected 2014-05-11.

@dcechano dcechano changed the title Operations on NaiveDate are not communative. Operations on NaiveDate are not commutative. Oct 29, 2023
@djc
Copy link
Contributor

djc commented Oct 30, 2023

This is expected, and documented:

An addition of months to NaiveDate clamped to valid days in resulting month.

Note the use of clamped.

@dcechano
Copy link
Contributor Author

Thank you for your reply. However, I'm not sure I understand. I saw this but it didn't appear to be relevant to my issue. Why would clamping of days result in operations being non-commutative?

@djc
Copy link
Contributor

djc commented Oct 30, 2023

Because NaiveDate::from_ymd(2023, 1, 31).add(Months::new(1)).add(Months::new(1)) is not the same as NaiveDate::from_ymd(2023,1, 31).add(Months::new(2)).

@dcechano
Copy link
Contributor Author

dcechano commented Oct 30, 2023

Why not? I suppose you are saying one results in clamping while the other doesn't? I'm sorry, I am still not sure I follow. In your example nothing is being commuted.

@djc
Copy link
Contributor

djc commented Oct 30, 2023

You can try the statements I posted in my last comment and see why not, right? I understand in my example there is no commutation but I think it shows that any expression containing add(Months) can never be commutative.

@dcechano
Copy link
Contributor Author

I appreciate you trying to help me. I won't be able to run your example until later this evening. For now, I can be satisfied with the problem being tied to how addition of months is implemented.

@pitdicker
Copy link
Collaborator

It is not really a question of how things are implemented in chrono. Working with dates is just tricky with all sorts of failure cases because months and years don't have a fixed number of days. (Let's forget the stuff when you add the time-of-day.)

We have Add and Sub implementations for convenience, but the domain just does not allow the operations to always be commutative. And as @djc said adding and subtracting Months does clamping instead of returning an error, which I suppose makes sense in some cases. Not my preference though.

Is there a specific problem you are trying to solve that we can help with?

@dcechano
Copy link
Contributor Author

dcechano commented Nov 1, 2023

Yes, I am building a parser that can take an arbitrary string such as "2022-05-15 + 1 Month -10 years + 10 hours" and give the resultant date back. The problem I ran into is that the date returned is in some cases dependant on the order of operations. I was tempted to ask what the "correct" order of operations was but I felt that it may be too arbitrary.

@pitdicker
Copy link
Collaborator

Ah, cool. You may want to read a bit about the plans in #1282.
It is sadly not ready yet, but shows the 'least bad' order of operations. Try to group things like year and month so they are added in one operation. The fewer different components you add, the more you avoid intermediate values that may not exist.

@dcechano
Copy link
Contributor Author

dcechano commented Nov 2, 2023

Try to group things like year and month so they are added in one operation. The fewer different components you add, the more you avoid intermediate values that may not exist.

Thank you! Grouping operations is very helpful advice. Having looked at #1282 it sounds like a great feature and may help with our issues over at coreutils.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants