GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimestamp() by nanjekyejoannah · Pull Request #15773 · python/cpython

Conversation

@nanjekyejoannah

@nanjekyejoannah nanjekyejoannah commented Sep 9, 2019 •
edited by miss-islington
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue37488

Automerge-Triggered-By: @pganssle

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nanjekyejoannah
nanjekyejoannah requested review from abalkin and pganssle as code owners September 9, 2019 12:46
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 9, 2019
@bedevere-bot bedevere-bot added the awaiting review label Sep 9, 2019
@pganssle

pganssle commented Sep 9, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pganssle
pganssle reviewed Sep 9, 2019
View reviewed changes
Comment thread Doc/library/datetime.rst Outdated

@pganssle pganssle Sep 9, 2019

Copy link
Copy Markdown
Member

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.

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....

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nanjekyejoannah

nanjekyejoannah commented Sep 9, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
epicfaace
epicfaace suggested changes Sep 10, 2019
View reviewed changes
Comment thread Doc/library/datetime.rst Outdated

@epicfaace epicfaace Sep 10, 2019

Copy link
Copy Markdown
Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aeros reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Misc/NEWS.d/next/Library/2019-09-09-12-48-10.bpo-37488.kteuwA.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Sep 10, 2019
@aeros aeros added the docs Documentation in the Doc dir label Sep 10, 2019
aeros
aeros reviewed Sep 10, 2019
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

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.

Thanks for the PR @nanjekyejoannah.

I have a few suggestions:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/datetime.rst Outdated

@aeros aeros Sep 10, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/datetime.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/library/datetime.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pganssle pganssle mentioned this pull request Sep 11, 2019
bpo-9004: Recommend against using utctimetuple in the docs. #10870
Closed
@pganssle

pganssle commented Sep 11, 2019

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nanjekyejoannah
nanjekyejoannah force-pushed the issue37488 branch from 2588293 to 2e5dc03 Compare September 11, 2019 11:42
aeros
aeros approved these changes Sep 11, 2019
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

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.

@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nanjekyejoannah @pganssle
Add warning notes in the documentation for utc* methods
84887b0 
This also removes several examples using utc* methods, to avoid
confusion.

bpo-37488: https://bugs.python.org/issue37488
@pganssle
pganssle force-pushed the issue37488 branch from 7d9e901 to 84887b0 Compare September 11, 2019 13:37
pganssle
pganssle approved these changes Sep 11, 2019
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Sep 11, 2019
@pganssle pganssle added the 🤖 automerge label Sep 11, 2019
@miss-islington

miss-islington commented Sep 11, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington
miss-islington merged commit 1a53c78 into python:master Sep 11, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label 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
@nanjekyejoannah @DinoV
bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimes…
9ee4794 
…tamp() (pythonGH-15773)

https://bugs.python.org/issue37488



Automerge-Triggered-By: @pganssle
@pganssle pganssle added the needs backport to 3.8 label Sep 12, 2019
@miss-islington

miss-islington commented Sep 12, 2019

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2019
@nanjekyejoannah @miss-islington
bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimes…
6df46f5 
…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

bedevere-bot commented Sep 12, 2019

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.8 label Sep 12, 2019
miss-islington added a commit that referenced this pull request Sep 12, 2019
@miss-islington @nanjekyejoannah
bpo-37488 : Document a warning for datetime.utcnow() and utcfromtimes…
307c5fe 
…tamp() (GH-15773)

https://bugs.python.org/issue37488

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

Co-authored-by: Joannah Nanjekye <[email protected]>
@abalkin abalkin mentioned this pull request Apr 10, 2022
datetime.utctimetuple() should not set tm_isdst flag to 0 #53250
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pganssle pganssle pganssle approved these changes
@abalkin abalkin Awaiting requested review from abalkin
+2 more reviewers
@epicfaace epicfaace epicfaace requested changes
@aeros aeros aeros approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

docs Documentation in the Doc dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@nanjekyejoannah @pganssle @miss-islington @bedevere-bot @epicfaace @aeros @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.