classification
Title: Add tzinfo= argument to datetime.combine
Type: enhancement Stage: resolved
Components: Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: SilentGhost, belopolsky, python-dev
Priority: normal Keywords: patch

Created on 2016-07-31 22:12 by belopolsky, last changed 2016-08-02 21:49 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue27661.diff belopolsky, 2016-07-31 22:58 review
issue27661-2.diff belopolsky, 2016-08-01 15:58 review
issue27661-3.diff belopolsky, 2016-08-01 16:59 review
Messages (5)
msg271751 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-07-31 22:12
Add an optional tzinfo argument to datetime.combine() so that

 datetime.combine(d, t, info)

returns the same object as

 datetime.combine(d, t).replace(tzinfo=info)

but without creating an intermediate naive instance.

Guido's LGTM: https://mail.python.org/pipermail/datetime-sig/2016-July/000993.html
msg271786 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-08-01 15:58
The second patch includes documentation changes and addresses review commments.
msg271787 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-08-01 16:00
From review comments:

Lib/datetime.py:1482: def combine(cls, date, time, tzinfo=True):
On 2016/08/01 08:47:14, SilentGhost wrote:
> This strikes me as an odd default value, why not use a more conventional None?

This is the same default as used in the .replace() methods.  Arguably, a proper sentinel value would be a better choice, but I think True delivers better performance.  In any case, this is not something I would change without consulting with the PyPy folks.

See <http://bugs.python.org/review/27661/diff/18026/Lib/datetime.py#newcode1482>.
msg271788 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2016-08-01 16:02
From review comments:

Lib/datetime.py:1482: def combine(cls, date, time, tzinfo=True):
On 2016/08/01 12:23:12, berkerpeksag wrote:
> On 2016/08/01 08:47:14, SilentGhost wrote:
> > This strikes me as an odd default value, why not use a more conventional
None?
> 
> Agreed. It would also be good to make it keyword-only.

tzinfo is not kw-only in the other constructors and I don't think it should be here.  Unlike "fold", tzinfo value is usually recognizable at the call site.  It is either called something like "tzinfo", "tz" or "New_York" or is a call such as 'tz.get('US/Eastern').

I would always prefer datetime.combine(d, t, tzinfo) to  datetime.combine(d, t, tzinfo=tzinfo).  datetime.combine(d, t, New_York) vs. datetime.combine(d, t, tzinfo=New_York) is a closer call, but still the first form is readable enough.


See <http://bugs.python.org/review/27661/diff/18026/Lib/datetime.py#newcode1482>.
msg271859 - (view) Author: Roundup Robot (python-dev) Date: 2016-08-02 21:49
New changeset adce94a718e3 by Alexander Belopolsky in branch 'default':
Closes #27661: Added tzinfo keyword argument to datetime.combine.
https://hg.python.org/cpython/rev/adce94a718e3
History
Date User Action Args
2016-08-02 21:49:36python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg271859

resolution: fixed
stage: commit review -> resolved
2016-08-01 16:59:51belopolskysetfiles: + issue27661-3.diff
2016-08-01 16:02:54belopolskysetmessages: + msg271788
2016-08-01 16:00:27belopolskysetmessages: + msg271787
stage: patch review -> commit review
2016-08-01 15:58:28belopolskysetfiles: + issue27661-2.diff

messages: + msg271786
2016-08-01 06:47:32SilentGhostsetnosy: + SilentGhost
2016-07-31 22:58:07belopolskysetfiles: + issue27661.diff
keywords: + patch
stage: patch review
2016-07-31 22:12:21belopolskycreate