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.

Sign up for GitHub

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

Jump to bottom

bpo-32417: Make timedelta arithmetic respect subclasses #10902

Merged
merged 7 commits into from
Feb 4, 2019
Merged

bpo-32417: Make timedelta arithmetic respect subclasses #10902

merged 7 commits into from
Feb 4, 2019

Conversation

pganssle
Copy link
Member

@pganssle pganssle commented Dec 4, 2018 •
edited by bedevere-bot
Loading

This fixes both bpo-35364 and bpo-32417. See this comment for a detailed rationale.

By changing timedelta over to returning the subclass it was added to, fromutc now also respects the subclass of the datetime being converted, and consequently so do all the tz=... forms of the alternate constructors.

This will fix many major headaches for people implementing datetime and date subclasses.

https://bugs.python.org/issue32417
https://bugs.python.org/issue35364

https://bugs.python.org/issue32417

@pganssle pganssle force-pushed the timedelta_return_datetime_subclass branch from 42cdcac to 627d4cb Compare December 7, 2018 16:31
vstinner
vstinner reviewed Dec 7, 2018
View reviewed changes
Lib/test/datetimetester.py Outdated Show resolved Hide resolved
vstinner
vstinner approved these changes Dec 7, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, but I would prefer to see a review from a second core dev. @serhiy-storchaka, @abalkin, @pablogsal maybe?

@pganssle pganssle force-pushed the timedelta_return_datetime_subclass branch from 627d4cb to 8c784f8 Compare January 2, 2019 19:23
@pganssle
Copy link
Member Author

pganssle commented Jan 2, 2019 •
edited
Loading

@vstinner Thanks for your comments, sorry for the delay in addressing them - I put it off for a bit because this change likely needs wider discussion and I haven't had time to have that discussion with python-dev before now.

I've addressed your style concerns, rebased against master and started a thread on python-dev for discussion.

@pganssle
Make timedelta return subclass types
c75a721 
Previously timedelta would always return the `date` and `datetime`
types, regardless of what it is added to. This makes it return
an object of the type it was added to.
@pganssle
Make pure python timedelta return subclass types
140d84f
@pganssle
Add tests for timedelta arithmetic on subclasses
4cf2b14
@pganssle
Add test for fromtimestamp with tz argument
11fde5b
@pganssle
Add tests for subclass behavior in now
e3d6747
@pganssle
Add news entry.
cdaa540 
Fixes:
bpo-32417
bpo-35364
@pganssle
More descriptive variable names in tests
598b665 
Addresses Victor's comments
@pganssle pganssle force-pushed the timedelta_return_datetime_subclass branch from 8c784f8 to 598b665 Compare February 4, 2019 14:48
gvanrossum
gvanrossum reviewed Feb 4, 2019
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

What about the C code?

@pganssle
Copy link
Member Author

pganssle commented Feb 4, 2019 •
edited
Loading

@gvanrossum That's the first commit, it's here (at the bottom of the diff). It's just a matter of switching to the C function that constructs either a datetime or subclass, depending on the type of self, which is why the diff is so small. This is done in a wrapper function because it's useful in several other places (and also is subject to optimization).

gvanrossum
gvanrossum approved these changes Feb 4, 2019
View reviewed changes
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Sorry I missed the C code at first! Reviewing on a phone is not ideal. All looks good. Go ahead and merge!

@abalkin abalkin self-assigned this Feb 4, 2019
@abalkin abalkin merged commit 89427cd into python:master Feb 4, 2019
@bedevere-bot
Copy link

@abalkin: Please replace # with GH- in the commit message next time. Thanks!

@pganssle pganssle mentioned this pull request Feb 8, 2019
Merged
@tirkarthi tirkarthi mentioned this pull request Jun 29, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

@abalkin abalkin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pganssle @bedevere-bot @vstinner @gvanrossum @abalkin @the-knights-who-say-ni