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

bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimestamp() #15773

Merged
merged 1 commit into from
Sep 11, 2019

Conversation

nanjekyejoannah
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah commented Sep 9, 2019

@pganssle
Copy link
Member

pganssle commented Sep 9, 2019

@nanjekyejoannah I don't think we can (or possibly should) put an actual warning in the code here - people will likely complain about it and we're not actually deprecating the function yet, it will likely just cause a lot of uproar and spam (as much as I'd like to...).

What I was referring to in bpo-15733 was a warning box in the documentation, like:

.. warning::

    Because naive datetimes are treated by many functions as local times,
    it is recommended that you create a timezone-aware datetime using
    ``datetime.now(tz=timezone.utc)`` instead.

I have not spent much time thinking about this. We may want a little footnote showing one of the gotchas to be referenced by both utcfromtimestamp and utcnow.

@@ -29,8 +29,9 @@ is used to represent a specific moment in time that is not open to
interpretation [#]_.

A naive object does not contain enough information to unambiguously locate
itself relative to other date/time objects. Whether a naive object represents
Coordinated Universal Time (UTC), local time, or time in some other timezone is
itself relative to other date/time objects. Native objects are no longer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this an intentional change? It looks like you kept in the beginnings of an explanation of the problem, but also the problematic old text.

This is actually a bit complicated to reword, because the sentiment behind it has changed, and we may need to hedge a bit. Let me think about how best to communicate the new intent....

@nanjekyejoannah
Copy link
Contributor Author

nanjekyejoannah commented Sep 9, 2019

Okay. I had thought you wanted us to introduce a warning. I will update this PR accordingly.

@nanjekyejoannah nanjekyejoannah changed the title bpo-37488 : Add warning to datetime.utcnow() and datetime.utcfromtimestamp() bpo-37488 : Document a warning for datetime.utcnow() Sep 9, 2019
@nanjekyejoannah nanjekyejoannah changed the title bpo-37488 : Document a warning for datetime.utcnow() bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimestamp() Sep 9, 2019
@@ -804,6 +811,11 @@ Other constructors, all class methods:
except the latter formula always supports the full years range: between
:const:`MINYEAR` and :const:`MAXYEAR` inclusive.

.. warning::

Because naive datetimes are treated by many functions as local times.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this wording to be of a similar sentence structure as in the earlier section (for datetime.utcnow())?

@aeros aeros added the docs Documentation in the Doc dir label Sep 10, 2019
Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @nanjekyejoannah.

I have a few suggestions:

@@ -752,6 +752,13 @@ Other constructors, all class methods:
:class:`.datetime` object. An aware current UTC datetime can be obtained by
calling ``datetime.now(timezone.utc)``. See also :meth:`now`.

.. warning::

Because naive datetimes are treated by many functions as local times,
Copy link
Contributor

@aeros aeros Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a bit more specific. Also, code comments or Sphinx roles should be used when referencing objects (such as classes, methods, or functions). In this case, code comments would be more appropriate since this is already within the section for datetime.

Suggested change
Because naive datetimes are treated by many functions as local times,
Because naive ``datetime`` objects are treated by many ``datetime`` methods as local times,

This line will need to be split off into the next one since it will exceed the reST maximum width of 80 chars (94 chars after the suggestion).

Doc/library/datetime.rst Outdated Show resolved Hide resolved
Doc/library/datetime.rst Outdated Show resolved Hide resolved
@pganssle
Copy link
Member

@nanjekyejoannah I believe this will have merge conflicts now that #13410 has been merged. Can you rebase against master? I also think adding a warning in utctimetuple() will allow us to close #10870.

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanjekyejoannah For consistency, can provide the warning message for each of them?

Because naive datetime objects are treated by many datetime methods
as local times, the recommended way to create aware datetime objects for
the current time in UTC is by calling datetime.now(timezone.utc).

Otherwise, everything else looks good to me.

This also removes several examples using utc* methods, to avoid
confusion.

bpo-37488: https://bugs.python.org/issue37488
@miss-islington
Copy link
Contributor

@nanjekyejoannah: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 1a53c78 into python:master Sep 11, 2019
@nanjekyejoannah nanjekyejoannah deleted the issue37488 branch September 11, 2019 14:33
DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019
@miss-islington
Copy link
Contributor

Thanks @nanjekyejoannah for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2019
…tamp() (pythonGH-15773)

https://bugs.python.org/issue37488

Automerge-Triggered-By: @pganssle
(cherry picked from commit 1a53c78)

Co-authored-by: Joannah Nanjekye <[email protected]>
@bedevere-bot
Copy link

GH-16059 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Sep 12, 2019
…tamp() (GH-15773)

https://bugs.python.org/issue37488

Automerge-Triggered-By: @pganssle
(cherry picked from commit 1a53c78)

Co-authored-by: Joannah Nanjekye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants