Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(239902)

#29100: datetime.fromtimestamp() doesn't check min/max year anymore: regression of Python 3.6

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 4 months ago by jordon.k.phillips
Modified:
2 years, 3 months ago
Reviewers:
storchaka+cpython, victor.stinner
CC:
lemburg, sasha, haypo, devnull_psf.upfronthosting.co.za, dstufft, elic_astllc.org, jordon.k.phillips_gmail.com
Visibility:
Public.

Patch Set 1 #

Total comments: 10

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/datetimetester.py View 1 1 chunk +36 lines, -0 lines 0 comments Download
Modules/_datetimemodule.c View 1 12 chunks +40 lines, -21 lines 0 comments Download

Messages

Total messages: 4
storchaka
http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py File Lib/test/datetimetester.py (right): http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py#newcode2014 Lib/test/datetimetester.py:2014: self.assertRaises(ValueError, self.theclass.fromtimestamp, ts) I didn't check, but I suppose ...
2 years, 3 months ago #1
victor.stinner_gmail.com
http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py File Lib/test/datetimetester.py (right): http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py#newcode2014 Lib/test/datetimetester.py:2014: self.assertRaises(ValueError, self.theclass.fromtimestamp, ts) On 2017/01/27 16:13:09, storchaka wrote: > ...
2 years, 3 months ago #2
storchaka
http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py File Lib/test/datetimetester.py (right): http://bugs.python.org/review/29100/diff/19640/Lib/test/datetimetester.py#newcode2014 Lib/test/datetimetester.py:2014: self.assertRaises(ValueError, self.theclass.fromtimestamp, ts) On 2017/01/27 16:35:46, haypo wrote: > ...
2 years, 3 months ago #3
victor.stinner_gmail.com
2 years, 3 months ago #4
http://bugs.python.org/review/29100/diff/19640/Modules/_datetimemodule.c
File Modules/_datetimemodule.c (right):

http://bugs.python.org/review/29100/diff/19640/Modules/_datetimemodule.c#newc...
Modules/_datetimemodule.c:4540: 
On 2017/01/27 16:55:17, storchaka wrote:
> Please don't do this. This is just a code churn.

I'm not a native english speaker, so I'm not sure about the meaning of "code
churn", but here I understand that you mean that the change is useless.

I like exiting a function as soon as possible, it prevents adding unnecessary
levels of indentation in the function.

I'm also trying slowly to put { ... } in if/else body to show more explicitly
the bounds of the code. Moreover, I understood that the PEP 7 suggests to use {
... }, even if I know that you understood it differently :-)

On this case, I don't feel confortable when I read the code because there is a
large block with lines at different indentation level (arguments written on
multiple lines), and it's not easy to see where the else block starts and where
the block ends. IMHO the modified code is easier to read.

Well, I don't want to argue for such tiny change. So ok, I will not push it.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+