bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python#4176
Conversation
|
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! |
|
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
left a comment
There was a problem hiding this comment.
LGTM, but I have a few comments.
| if day is None: | ||
| day = self._day | ||
| return date(year, month, day) | ||
| return type(self)(year, month, day) |
There was a problem hiding this comment.
I prefer self.__class__, but I never know which one is the best :-)
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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__...
| self.assertRaises(ValueError, base.replace, year=2001) | ||
|
|
||
| def test_subclass_replace(self): | ||
| class C(self.theclass): |
There was a problem hiding this comment.
Would you mind to rename the class to "DateSubclass" or anything longer than a single letter? :-) Same request for the class below.
There was a problem hiding this comment.
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?
|
I updated the subclass names to be somewhat more descriptive. The only remaining question is whether this needs a news item. |
There was a problem hiding this comment.
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.
0615f2b to
052302d
Compare
052302d to
8f26dd3
Compare
|
This should be ready to go - any thoughts on whether it needs NEWS? |
|
I merged your PR, thanks! |
…Pure Python (pythonGH-4176) (cherry picked from commit 191e993)
|
GH-4356 is a backport of this pull request to the 3.6 branch. |
Per the discussion on the issue tracker, the behavior of the pure python implementation of
datetime,dateandtimeare 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 inpypy3has been fixed for some time.https://bugs.python.org/issue31222