classification
Title: datetime.py implementation of .replace inconsistent with C implementation
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: p-ganssle, r.david.murray, steven.daprano, vstinner
Priority: normal Keywords: patch

Created on 2017-08-16 17:01 by p-ganssle, last changed 2017-11-10 10:21 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4176 merged p-ganssle, 2017-10-30 17:08
PR 4356 merged python-dev, 2017-11-09 21:36
Messages (8)
msg300377 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2017-08-16 17:01
In the .py implementation of datetime.replace (and date.replace and time.replace), the new datetime is created using the datetime type:

https://github.com/python/cpython/blob/master/Lib/datetime.py#L1578

But in the C source, it is created from type(self):

https://github.com/python/cpython/blob/master/Modules/_datetimemodule.c#L5046

I think the second should be the preferred behavior, so the datetime.py source should be updated to reflect that it's calling self.__class__(...) rather than datetime(...). I can prepare a PR if this would be desirable.

(That said, I'm not 100% clear under what circumstances the code in datetime.py is actually *used*, so I'm not sure how to write tests for it - is datetime.py essentially documentation, or is there a way to explicitly fall back to it?)

Per this issue on the pypy3 tracker:

https://bitbucket.org/pypy/pypy/issues/2635/datetimereplace-always-returns
msg300378 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2017-08-16 17:13
I agree that returning type(self) or self.__class__ (not sure which is better) is the right thing to do.

It might be possible to argue that the Python version is buggy, if the C version is treated as the reference implementation and the Python version has to duplicate that. Or if the documentation specifies the C version's behaviour.

But if the precise behaviour is unspecified, or if the Python version is the reference implementation, its best to treat this as an enhancement rather than a bug fix and only apply it to 3.7.

I'm not sure which is the right approach here.
msg300379 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-08-16 17:15
See also issue 20371.
msg300382 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2017-08-16 17:39
@r.david.murray In the other thread, you mention that the full test suite is run against the C and Python implementations, so that answers the question of how to write the tests.

I think treating it as an enhancement in Python 3.7 makes a reasonable amount of sense - it's clearly under-specified at the moment and people are probably relying on the CPython behavior (dateutil definitely is in the latest stable release, but not on master). Saying "it's implementation-specific before Python 3.7 but in Python 3.7+, the spec says it should use self(type)" is fine by me. It's not particularly hard to work around if you're subclassing datetime anyway.

Among the major libraries that provide their own datetime objects:

- Arrow seems to use composition (https://github.com/crsmithdev/arrow/blob/master/arrow/arrow.py)
- pendulum subclasses, but implements their own "replace": https://github.com/sdispater/pendulum/blob/master/pendulum/pendulum.py#L25
- delorean uses composition: https://github.com/myusuf3/delorean/blob/master/delorean/dates.py#L174
- maya uses composition: https://github.com/kennethreitz/maya/blob/master/maya/core.py#L72

I'd say for the most part it's not a major issue to change it even as a bugfix, particularly if the line we're going with is "it was always implementation-specific", but there's also no rush.
msg305240 - (view) Author: Paul Ganssle (p-ganssle) * (Python committer) Date: 2017-10-30 17:11
Some time ago this was fixed in pypy3:

https://bitbucket.org/pypy/pypy/issues/2635/datetimereplace-always-returns

I made a PR fixing this for `datetime`, `date` and `time`.
msg305984 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-09 21:34
New changeset 191e993365ac3206f46132dcf46236471ec54bfa by Victor Stinner (Paul Ganssle) in branch 'master':
bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python (#4176)
https://github.com/python/cpython/commit/191e993365ac3206f46132dcf46236471ec54bfa
msg306000 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-10 00:52
New changeset b9a40aca2935d2569191844c88f8b61269e383cb by Victor Stinner (Miss Islington (bot)) in branch '3.6':
bpo-31222: Make (datetime|date|time).replace return subclass type in Pure Python (GH-4176) (#4356)
https://github.com/python/cpython/commit/b9a40aca2935d2569191844c88f8b61269e383cb
msg306017 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-10 10:21
Thanks Paul Ganssle for the bugfix! I merged your PR and backported it to Python 3.6. (Python 3.5 doesn't accept bugfixes anymore.)
History
Date User Action Args
2018-12-04 17:21:38belopolskylinkissue20371 superseder
2017-11-10 10:21:22vstinnersetstatus: open -> closed
versions: - Python 3.5
messages: + msg306017

resolution: fixed
stage: patch review -> resolved
2017-11-10 00:52:07vstinnersetmessages: + msg306000
2017-11-09 21:36:08python-devsetpull_requests: + pull_request4312
2017-11-09 21:34:35vstinnersetnosy: + vstinner
messages: + msg305984
2017-10-30 17:11:14p-gansslesetmessages: + msg305240
2017-10-30 17:08:07p-gansslesetkeywords: + patch
stage: patch review
pull_requests: + pull_request4145
2017-08-16 17:39:48p-gansslesetmessages: + msg300382
2017-08-16 17:15:22r.david.murraysetnosy: + r.david.murray
messages: + msg300379
2017-08-16 17:13:13steven.dapranosetnosy: + steven.daprano
messages: + msg300378
2017-08-16 17:01:45p-gansslecreate