bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python #4176

pganssle · 2017-10-30T17:08:06Z

Per the discussion on the issue tracker, the behavior of the pure python implementation of datetime, date and time are out of whack with the equivalent from _datetimemodule.c. Since the C version is almost certainly what's being used almost everywhere, this shouldn't have any real behavioral changes. The equivalent bug in pypy3 has been fixed for some time.

https://bugs.python.org/issue31222

the-knights-who-say-ni · 2017-10-30T17:08:08Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

pganssle · 2017-10-30T17:08:46Z

I'll note that since there's no actual behavior change for the delivered binaries, it's not clear whether this requires a NEWS entry.

vstinner

LGTM, but I have a few comments.

vstinner · 2017-10-30T17:26:21Z

Lib/datetime.py

@@ -828,7 +828,7 @@ def replace(self, year=None, month=None, day=None):
            month = self._month
        if day is None:
            day = self._day
-        return date(year, month, day)
+        return type(self)(year, month, day)


I prefer self.__class__, but I never know which one is the best :-)

I don't have any real preference here. I usually go with self.__class__ myself, but I was somewhat persuaded by the discussion on StackOverflow that seemed to indicate that they are equivalent in Python 3, and that type is shorter - seemed like a mixed bag as to which one is more intuitive. I'm happy to change it to the "house style".

Not sure if it's even a relevant precedent, since they're in two different languages, but it does seem that the C source code uses Py_TYPE(self) rather than some equivalent of self.__class__. Though for all I know that's a macro that expands to self.__class__...

vstinner · 2017-10-30T17:27:43Z

Lib/test/datetimetester.py

@@ -1520,6 +1520,13 @@ def test_replace(self):
        base = cls(2000, 2, 29)
        self.assertRaises(ValueError, base.replace, year=2001)

+    def test_subclass_replace(self):
+        class C(self.theclass):


Would you mind to rename the class to "DateSubclass" or anything longer than a single letter? :-) Same request for the class below.

I don't have particularly strong opinions on the naming here, I mainly used C because test_subclass_date and test_subclass_time both use C as their "in-function subclass", and there are already top-level SubclassTime/SubclassDate/SubclassDatetime classes that are used to run all or nearly all the tests again on a subclass (presumably to check that subclasses satisfy the same contract as the base class). I think DateSubclass might get confused with these other test classes.

Another alternative which avoids this question entirely is to replace the specific test_subclass_replace function with a test_replace_class function (which will automatically get the subclass check by virtue of the inheritance through TestSubclassDateTime). The main reasons I didn't do this are that 1. I think the logic is a bit clearer (why are you checking that .replace returns the same class as the object?) and 2. this way it automatically "recurses" the check such that it's clear that even subclasses retain this property. This is a fairly weakly-held opinion, though, and I'm willing to be changed.

If we do decide that renaming is best, would maybe _Subclass be a reasonable name?

pganssle · 2017-11-05T19:26:08Z

I updated the subclass names to be somewhat more descriptive. ~~I'm not sure if the bot is supposed to automatically update when my CLA goes through, but I've been told that the signed CLA I sent in has been processed.~~

The only remaining question is whether this needs a news item.

vstinner · 2017-11-06T09:32:30Z

Lib/test/datetimetester.py

@@ -1520,6 +1520,13 @@ def test_replace(self):
        base = cls(2000, 2, 29)
        self.assertRaises(ValueError, base.replace, year=2001)

+    def test_subclass_replace(self):
+        class _DateSubclass(self.theclass):


Please remove "_" prefix in class names. These classes are only defined in the scope of the test, no need to declare them as "private" with the "_" prefix.

pganssle · 2017-11-09T16:14:56Z

This should be ready to go - any thoughts on whether it needs NEWS?

vstinner · 2017-11-09T21:34:38Z

I merged your PR, thanks!

miss-islington · 2017-11-09T21:34:59Z

Thanks @pganssle for the PR, and @Haypo for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

…Pure Python (pythonGH-4176) (cherry picked from commit 191e993)

bedevere-bot · 2017-11-09T21:36:09Z

GH-4356 is a backport of this pull request to the 3.6 branch.

…Pure Python (GH-4176) (#4356) (cherry picked from commit 191e993)

pganssle added 2 commits October 30, 2017 11:54

Add failing test for pure Python datetime subclasses

511e2f1

Bring pure Python (datetime|date|time).replace behavior in line with C

dea1eaf

the-knights-who-say-ni added the CLA not signed label Oct 30, 2017

bedevere-bot added the awaiting review label Oct 30, 2017

vstinner reviewed Oct 30, 2017

View reviewed changes

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Nov 5, 2017

vstinner reviewed Nov 6, 2017

View reviewed changes

pganssle force-pushed the datetime_type branch from 0615f2b to 052302d Compare November 6, 2017 11:11

Use more descriptive subclass names in subclass_replace

8f26dd3

pganssle force-pushed the datetime_type branch from 052302d to 8f26dd3 Compare November 6, 2017 14:23

vstinner added the skip news label Nov 9, 2017

vstinner merged commit 191e993 into python:master Nov 9, 2017

bedevere-bot removed the awaiting review label Nov 9, 2017

vstinner added the needs backport to 3.6 label Nov 9, 2017

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 9, 2017

bpo-31222: Make (datetime|date|time).replace return subclass type in …

c1904fb

…Pure Python (pythonGH-4176) (cherry picked from commit 191e993)

bedevere-bot removed the needs backport to 3.6 label Nov 9, 2017

vstinner pushed a commit that referenced this pull request Nov 10, 2017

bpo-31222: Make (datetime|date|time).replace return subclass type in …

b9a40ac

…Pure Python (GH-4176) (#4356) (cherry picked from commit 191e993)

pganssle deleted the datetime_type branch October 25, 2018 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python #4176

bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python #4176

Uh oh!

pganssle commented Oct 30, 2017 •

edited by bedevere-bot

Loading

Uh oh!

the-knights-who-say-ni commented Oct 30, 2017

Uh oh!

pganssle commented Oct 30, 2017

Uh oh!

vstinner left a comment

Uh oh!

vstinner Oct 30, 2017 •

edited

Loading

Uh oh!

pganssle Oct 30, 2017

Uh oh!

pganssle Oct 30, 2017

Uh oh!

vstinner Oct 30, 2017

Uh oh!

pganssle Oct 30, 2017

Uh oh!

pganssle commented Nov 5, 2017 •

edited

Loading

Uh oh!

vstinner Nov 6, 2017 •

edited

Loading

Uh oh!

pganssle commented Nov 9, 2017

Uh oh!

vstinner commented Nov 9, 2017

Uh oh!

miss-islington commented Nov 9, 2017

Uh oh!

bedevere-bot commented Nov 9, 2017

Uh oh!

Uh oh!

Uh oh!

bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python #4176

bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python #4176

Uh oh!

Conversation

pganssle commented Oct 30, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Oct 30, 2017

Uh oh!

pganssle commented Oct 30, 2017

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle commented Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle commented Nov 9, 2017

Uh oh!

vstinner commented Nov 9, 2017

Uh oh!

miss-islington commented Nov 9, 2017

Uh oh!

bedevere-bot commented Nov 9, 2017

Uh oh!

Uh oh!

pganssle commented Oct 30, 2017 •

edited by bedevere-bot

Loading

vstinner Oct 30, 2017 •

edited

Loading

pganssle commented Nov 5, 2017 •

edited

Loading

vstinner Nov 6, 2017 •

edited

Loading