-
Notifications
You must be signed in to change notification settings - Fork 778
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
Fix fixed offset timezone conversion bug. #3269
Fix fixed offset timezone conversion bug. #3269
Conversation
Since we are going through a license change, please also have a loot at #3108 |
Not sure what the Windows build failures are about. Is the Python installation missing the time zone database there? |
It's because Windows only supports times in 1970-2038 I believe, but the proptest is using a much larger range, I am fixing this now |
Hunting down some more bugs the more exhaustive proptest found. Found one in chrono: chronotope/chrono#1154 There is another one that results in the python vs rust datetimes being 1 second off when the time falls exactly on the daylight savings switchover. Unsure if pyo3 conversion bug (with the |
@grantslatton I am interested in the issues you may run into. |
@pitdicker Does chrono have facilities for supporting ambiguous instants created at the end of daylight savings? i.e. https://peps.python.org/pep-0495/ |
Yes, you will want to look into methods that return a |
0a8c978
to
139516c
Compare
I am wondering if we should also fix #3266 in the same change, because this change will result in But doing this would require changing the I think such a change increases the correctness of this crate, because right now the conversion is lossy and loses daylight savings information present in |
This would mean the fix would also be limited to 0.20, the next release where breakage is possible. So I think the question is whether the side effects of losing the fold information is serious enough that one would say the backwards-compatible version of the fix cannot be used anyway? Personally, I would be fine with try to fix this properly for 0.20 only because the chrono integration is still a relatively new feature of PyO3 and some churn seems expected. |
P.S.: You seem to have included two unrelated commits during your rebase. |
139516c
to
b1e7ed8
Compare
Ok, sorry for the delay. I've reviewed the issue and the PR, the bug is there, and this fixes it, nice catch.
The problem with the All that to say that it's probably ok to lose the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@grantslatton @Psykopear Ok, you to summarize my understanding: We take as it is and it fixes #3267 but #3266 will need a more involved approach which might even use an independent code path. Do you both agree with this?
Yes this is right |
Closes #3267