NotificationsYou must be signed in to change notification settings
Fork32.4k
Star67.9k

gh-72346: Added isdst deprecation warning to email.utils.localtime#91450

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

zware merged 39 commits intopython:mainfromalanfwilliams:fix-gh-72346

Mar 20, 2023

Merged

gh-72346: Added isdst deprecation warning to email.utils.localtime#91450

zware merged 39 commits intopython:mainfromalanfwilliams:fix-gh-72346

Mar 20, 2023

Conversation

Copy link

Contributor

alanfwilliams commentedApr 11, 2022•
edited by bedevere-bot
Loading

This adds a deprecation warning to the isdst parameter in email.utils.localtime. This is my first contribution so any feedback is welcome.

Issue:Deprecate isdst argument in email.utils.localtime #72346

Added isdst deprecation warning

dadf5e2

alanfwilliams requested a review froma team as acode owner

April 11, 2022 18:31

bedevere-bot added the awaiting review label

Apr 11, 2022

📜🤖 Added by blurb_it.

77414f9

This comment was marked as outdated.

Copy link

ContributorAuthor

alanfwilliams commentedApr 11, 2022

I'm not sure why it says I haven't signed the CLA. I did just link it with my github account so it may take a moment to refresh I assume.

Copy link

Member

zware commentedApr 11, 2022

Closing and re-opening to re-trigger the CLA check.

zware closed this

Apr 11, 2022

zware reopened this

Apr 11, 2022

zware linked an issue

Apr 12, 2022

that may beclosed by this pull request

Deprecate isdst argument in email.utils.localtime#72346

Closed

hugovk reviewed

Apr 12, 2022

View reviewed changes

Copy link

Member

hugovk left a comment

Choose a reason for hiding this comment

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

A good start, thanks!

It would need some documentation updating too.

Put a.. deprecated:: 3.11 under the function inemail.utils.rst. For example, have a look at:

And also list it on "What’s New In Python 3.11":

Lib/email/utils.py OutdatedShow resolvedHide resolved

added documentation and used _deprecated

add07f3

Copy link

ContributorAuthor

alanfwilliams commentedApr 12, 2022

@hugovk I've added the edits you've requested. Thanks for the quick review.

Removed trailing whitespace.

854ebf8

hugovk reviewed

Apr 12, 2022

View reviewed changes

Copy link

Member

hugovk 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 update!

Doc/whatsnew/3.11.rst OutdatedShow resolvedHide resolved

Doc/library/email.utils.rst OutdatedShow resolvedHide resolved

Lib/email/utils.py OutdatedShow resolvedHide resolved

added newline, moved whatsnew, reformatted warning

1b9a60f

Copy link

ContributorAuthor

alanfwilliams commentedApr 13, 2022

@hugovk I've completed the changes you requested.

hugovk reviewed

Apr 13, 2022

View reviewed changes

Copy link

Member

hugovk left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple of minor style things!

Doc/library/email.utils.rst Outdated

		@@ -26,6 +26,8 @@ module:

		.. versionadded:: 3.3

		.. deprecated:: 3.11
		Use of the ``isdst`` argument is deprecated.

Copy link

Member

Choose a reason for hiding this comment

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

I just noticed the style for parameters is italics (for example see the docstring):

Suggested change

	Use of the``isdst`` argument is deprecated.
	Use of theisdst argument is deprecated.

Doc/whatsnew/3.11.rst Outdated

		@@ -853,6 +853,9 @@ Deprecated

		(Contributed by Brett Cannon in :issue:`47061`.)

		* Deprecated the ``isdst`` parameter in :func:`email.utils.localtime`.

Copy link

Member

Choose a reason for hiding this comment

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

Suggested change

	* Deprecated the``isdst`` parameter in:func:`email.utils.localtime`.
	* Deprecated theisdst parameter in:func:`email.utils.localtime`.

Lib/email/utils.py Outdated

		@@ -345,6 +346,8 @@ def localtime(dt=None, isdst=-1):
		to divine whether summer time is in effect for the specified time.

		"""
		if isdst != -1:
		warnings._deprecated("The isdst parameter to localtime", remove=(3,13))

Copy link

Member

Choose a reason for hiding this comment

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

A little nit :)

Suggested change

	warnings._deprecated("The isdst parameter to localtime",remove=(3,13))
	warnings._deprecated("The isdst parameter to localtime",remove=(3,13))

formatting corrections

6784b42

hugovk approved these changes

Apr 13, 2022

View reviewed changes

Copy link

Member

hugovk left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Thanks, looks good! Hopefully a core dev can take a look soon and trigger the CI.

cc@abalkin,@bitdancer,@Mariatta

bedevere-bot added awaiting core review and removed awaiting review labels

Apr 13, 2022

hugovk added topic-email stdlib

Python modules in the Lib dir

3.11only security fixes labels

Apr 13, 2022

Copy link

ContributorAuthor

alanfwilliams commentedApr 26, 2022

Hi, just wanted to check if there was anything I could do to move this along.

zware requested changes

Apr 26, 2022

View reviewed changes

Copy link

Member

zware left a comment

Choose a reason for hiding this comment

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

This is a great start! A few things to fix up, though.

There will also need to be test changes for this, both to test that the deprecation warning is emitted as expected, and to prevent it from escaping where we don't want it.

Also, note that I'm not a maintainer of theemail package, so these are just general suggestions to hopefully make this quicker and easier for one of them to accept :)

Lib/email/utils.py Outdated

		@@ -345,6 +346,8 @@ def localtime(dt=None, isdst=-1):
		to divine whether summer time is in effect for the specified time.

		"""
		if isdst != -1:

Copy link

Member

Choose a reason for hiding this comment

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

We need to change the default value here to detect when someone is passing in-1 explicitly.None is fine as the new default, or you could add_ignored = object() just before this function and use_ignored as the new default.

Lib/email/utils.py Outdated

		@@ -345,6 +346,8 @@ def localtime(dt=None, isdst=-1):
		to divine whether summer time is in effect for the specified time.

		"""
		if isdst != -1:
		warnings._deprecated("The isdst parameter to localtime", remove=(3,13))
		if dt is None:
		return datetime.datetime.now(datetime.timezone.utc).astimezone()
		if dt.tzinfo is not None:

Copy link

Member

zwareApr 13, 2022

Choose a reason for hiding this comment

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

As mentioned in the issue, this whole function can be reduced to just a thin wrapper arounddatetime.astimezone():

ifdtisNone:dt=datetime.datetime.now()returndt.astimezone()

(Extra code for the deprecated parameter not shown :))

Lib/email/utils.py Outdated

		@@ -29,6 +29,7 @@
		import socket
		import datetime
		import urllib.parse
		import warnings

Copy link

Member

zwareApr 13, 2022

Choose a reason for hiding this comment

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

Since this should generally not be necessary unless someone is passing in a value forisdst, it can be imported where it's used.

Doc/library/email.utils.rst Outdated

		@@ -26,6 +26,8 @@ module:

		.. versionadded:: 3.3

		.. deprecated:: 3.11

Copy link

Member

zwareApr 13, 2022

Choose a reason for hiding this comment

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

Since we're specifying in the code that it will be removed in 3.13, we can use thedeprecated-removed directive here to also document when it will disappear.

Lib/email/utils.py Outdated

		@@ -345,6 +346,8 @@ def localtime(dt=None, isdst=-1):
		to divine whether summer time is in effect for the specified time.

Copy link

Member

zwareApr 13, 2022

Choose a reason for hiding this comment

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

The docstring should be updated to remove everything aboutisdst, and replace it with a simple note that the parameter is ignored. Similar in the documentation.

Lib/email/utils.py Outdated

		@@ -345,6 +346,8 @@ def localtime(dt=None, isdst=-1):
		to divine whether summer time is in effect for the specified time.

		"""
		if isdst != -1:
		warnings._deprecated("The isdst parameter to localtime", remove=(3, 13))

Copy link

Member

zwareApr 16, 2022

Choose a reason for hiding this comment

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

Unfortunately, the easy route here makes for an ugly message:'The isdst argument to localtime' is deprecated .... We need to specify our own message to make it prettier:

Suggested change

	warnings._deprecated("The isdst parameter to localtime",remove=(3,13))
	warnings._deprecated(
	"The 'isdst' parameter to 'localtime'",
	message='{name}isdeprecatedandslatedforremovalinPython {remove},
	remove=(3,13),
	)

Doc/library/email.utils.rst Outdated

		@@ -11,7 +11,7 @@
		There are a couple of useful utilities provided in the :mod:`email.utils`
		module:

		.. function:: localtime(dt=None)
		.. function:: localtime(dt=None, isdst=-1)

Copy link

Member

zwareApr 26, 2022

Choose a reason for hiding this comment

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

Since it wasn't documented before and is being removed, we probably don't want to add it now :). The deprecation note can mention that it was previously undocumented.

Doc/whatsnew/3.11.rst Outdated

		@@ -853,6 +853,9 @@ Deprecated

		(Contributed by Brett Cannon in :issue:`47061`.)

		* Deprecated the isdst parameter in :func:`email.utils.localtime`.
		(Contributed by Alan Williams in :issue:`72346`.)

Copy link

Member

zwareApr 26, 2022

Choose a reason for hiding this comment

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

:issue:`72346` will direct tohttps://bugs.python.org/issue72346, which doesn't exist :). This should now be:gh:`72346`, IIUC.

bedevere-bot added awaiting changes and removed awaiting core review labels

Apr 26, 2022

Copy link

bedevere-bot commentedApr 26, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ezio-melotti reviewed

Aug 30, 2022

View reviewed changes

Lib/email/utils.py OutdatedShow resolvedHide resolved

ezio-melottiand others added6 commits

August 30, 2022 23:23

Revert incorrect change.

9389791

Merge branch 'fix-pythongh-72346' ofhttps://github.com/aw1231/cpython…

672b5e5

…into fix-pythongh-72346

Merge branch 'main' into fix-pythongh-72346

89eb11a

Merge branch 'fix-pythongh-72346' ofhttps://github.com/aw1231/cpython…

c2b4c94

…into fix-pythongh-72346

Modified existing tests/added test for deprecation

31a4c46

Merge branch 'main' into fix-pythongh-72346

54596ef

hugovk reviewed

Sep 8, 2022

View reviewed changes

Doc/whatsnew/3.12.rst OutdatedShow resolvedHide resolved

alanfwilliams added2 commits

September 8, 2022 10:26

Moved whatsnew note

d0eda9d

Merge branch 'fix-pythongh-72346' ofhttps://github.com/aw1231/cpython…

ecc3bff

…into fix-pythongh-72346

Copy link

ContributorAuthor

alanfwilliams commentedSep 8, 2022

Although it seems that the Azure test isn't working properly (says it passed in the output, but is marked as failed), I have made the requested changes; please review again.