Issue27661
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) * ![]() |
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) * ![]() |
Date: 2016-08-01 15:58 | |
The second patch includes documentation changes and addresses review commments. |
|||
| msg271787 - (view) | Author: Alexander Belopolsky (belopolsky) * ![]() |
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) * ![]() |
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:36 | python-dev | set | status: open -> closed nosy:
+ python-dev resolution: fixed |
| 2016-08-01 16:59:51 | belopolsky | set | files: + issue27661-3.diff |
| 2016-08-01 16:02:54 | belopolsky | set | messages: + msg271788 |
| 2016-08-01 16:00:27 | belopolsky | set | messages:
+ msg271787 stage: patch review -> commit review |
| 2016-08-01 15:58:28 | belopolsky | set | files:
+ issue27661-2.diff messages: + msg271786 |
| 2016-08-01 06:47:32 | SilentGhost | set | nosy:
+ SilentGhost |
| 2016-07-31 22:58:07 | belopolsky | set | files:
+ issue27661.diff keywords: + patch stage: patch review |
| 2016-07-31 22:12:21 | belopolsky | create | |

