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

Add AddAssign/SubAssign implementation to DateTime/Date #698

Merged
merged 6 commits into from
Jun 11, 2022

Conversation

MrGunflame
Copy link
Contributor

This implements the AddAsssign and SubAssign traits for the non-naive DateTime and Date structs.

@djc
Copy link
Member

djc commented May 24, 2022

I'm not excited about this change. For one thing, I question the utility of the Date<Tz> type, which seems inherently odd to me, and fragile, since it's such a weird idea of having a date without time but with timezone. For another, these implementations are bypassing checked_{add,sub}_signed() and only changing the internal NaiveDateTime -- what's the behavior in the presence of an offset change, does that behavior align with the Add implementation? Also there are no tests for any of this.

@MrGunflame
Copy link
Contributor Author

MrGunflame commented May 24, 2022

I don't know how useful the Date<Tz> type is, but I included a {Add,Sub}Assign anyways because it is in a similar to DateTime in that regard. Further changing the NaiveDateTime directly shouldn't cause problems when changing the offset since it is always UTC and the time changes are always relative. I will add some tests to validate that the behavior aligns with the Add/Sub implementation later.

@esheppa
Copy link
Collaborator

esheppa commented Jun 3, 2022

@MrGunflame - this should delegate to the underlying checked_add_signed functions or copy their implementation as for certain types (eg Local) the information to work out the offset isn't stored in the type itself and only the offset itself is stored. Here the behavior of checked_add_signed causes the offset to be updated if required by first adding to the underlying (utc) NaiveDateTime, and then calling tz.from_utc_datetime on the resulting NaiveDateTime. Ideally the tests could be expanded to include one with Local which when tested in CI would try it out with a few different timezones - in this case the duration added would need to be sufficiently long to ensure a timezone change is passed.

@MrGunflame
Copy link
Contributor Author

Thanks, I tried to avoid checked_add_signed since calling it not really an option, so copying the implementation is the only way. Also interestingly Date does not use the equivalent tz.from_utc_date method and simply adds the new duration onto the NaiveDate in its checked_add_signed implementation.

@esheppa
Copy link
Collaborator

esheppa commented Jun 4, 2022

Interesting point! On the surface it looks like the idea is that it is not necessary on Date<Tz> because once and_time is called (or any equivalents eg. and_hms*, then it goes through from_local_datetime. However, this is still an issue - if someone were to query the offset prior to converting to DateTime then it might be wrong! I think the best solution for now would be to match existing behavior and replicate the impls. It looks like longer term in a 0.5 release the Date<Tz> may be removed anyway, resolving the potential issue with offsets.

@esheppa
Copy link
Collaborator

esheppa commented Jun 9, 2022

Thanks @MrGunflame the changes look good to me! I've just set up a small PR to this branch on your fork with some suggested changes to the Local tests to ensure we are testing that behavior matches across DST transitions

@djc
Copy link
Member

djc commented Jun 9, 2022

@esheppa would you be interested in becoming a collaborator?

@esheppa
Copy link
Collaborator

esheppa commented Jun 9, 2022

@djc - for sure, happy to do so

@esheppa
Copy link
Collaborator

esheppa commented Jun 10, 2022

Hi @MrGunflame this is now looking great - if possible could you please rebase on the main branch so CI can run here? Once it has succeeded I'm happy to merge this.

@MrGunflame
Copy link
Contributor Author

Should be good to go now.

@esheppa
Copy link
Collaborator

esheppa commented Jun 11, 2022

Thanks @MrGunflame

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

Successfully merging this pull request may close these issues.

3 participants