-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DOC: timezone warning for dates beyond TODAY #34100
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
Conversation
introducing a suggestion discussed in PR #33863 : Added a warning in the user guide that timezone conversion on future dates is inherently unreliable.
@@ -2314,13 +2314,24 @@ you can use the ``tz_convert`` method. | |||
Instead, the datetime needs to be localized using the ``localize`` method | |||
on the ``pytz`` time zone object. | |||
|
|||
.. warning:: | |||
|
|||
Be aware that for times in the future, correct conversion between time zones |
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.
I think instead of adding a new warning box, the last sentence of the warning box below should be Be aware that for times in the future, correct conversion between time zones (and UTC) cannot be guaranteed by any time zone library
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.
I feel strongly that this should be a separate warning. These are two different issues (even though the solution is basically the same), and one sentence could easily be overlooked in the big 2038 warning. Here's my rationale as I said int he discussion at #33863 :
Okay. I'm convinced now. We should warn about converting (any) future time. I still think this warning shouldn't be an afterthought in the 2038 warning text. It should be its own warning box. That gives it the prominence it deserves and ensures the warning remains if the 2038 problem is resolved in the downstream libraries and that box is removed.
In addition, the example given in the warning below only concerns the 2038 problem. How is anyone reading the docs supposed to know that the example only concerns the first part of the warning (the 2038 problem) but not the last sentence. It's just confusing that way.
What do you think about adding the warning I proposed but only with the first sentence?
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.
Okay maybe you can combine this warning and the 2038 warning into 1 warning, highlighting your point first and the 2038 issue as a ramification.
Just want to reduce the amount of warnings in this section since there's already a lot of them.
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.
How is a longer warning better than two warnings? Of the five warnings here, the first and second warnings about differing timezone definitions between libraries and library versions respectively are also not merged into one. For good reason, I would say. Keeping five separate warnings makes the structure quickest to comprehend and hence the user guide easiest to read, which I think is the ultimate goal. That's why there should be a separate, succinct warning about time zone conversions for future dates.
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.
I would agree this can at least be shortened a bit. We don't need to single out Morocco
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.
I just committed a change to make it shorter but CI / Web and Docs throws: AttributeError: module 'numpy.random' has no attribute 'BitGenerator'
. I have no idea what that means.
what is the status here? |
LGTM. Just looks like master needs to be merged in |
I don't have a working dev environment and no time to set it up. I also tried to figure out if this can be done on Github only (without using my local machine) but couldn't find a way to merge master into my GH repository online. Feel free to take over the PR and fix the one CI error it still throws. Or I'll get to it when I get to it -- that is sometime this year. |
I merged in master but there are again CI failures unrelated to the docs. Why is it so hard to get a simple doc change in? |
Have merged master, CI passes now - still happy with it @mroeschke ? |
Thanks @joooeey ! |
introducing a suggestion discussed in PR #33863 :
Added a warning in the user guide that timezone conversion on future dates is inherently unreliable.
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff