classification
Title: Added description for assert statement
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, terry.reedy, thatiparthy
Priority: normal Keywords: patch

Created on 2013-11-12 17:40 by thatiparthy, last changed 2013-12-13 21:25 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
datetime.patch thatiparthy, 2013-11-12 17:40 review
Messages (6)
msg202708 - (view) Author: Srinivas Reddy Thatiparthy(శ్రీనివాస్ రెడ్డి తాటిపర్తి) (thatiparthy) * Date: 2013-11-12 17:40
Added descriptive message to assert statement in datetime module.
Since _check_date_fields does the job of data integrity, i did not check for ValueError, TypeError checks in the function.

However, i am not sure of the adding descriptive messages to the other assert statements like, assert seconds == int(seconds). And isn't this too much defensive programming?
msg202998 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-11-16 00:57
Since failed asserts print the failed assert, repeating the assertion in a message is useless.
>>> assert 1 <= i
Traceback (most recent call last):
  File "<pyshell#3>", line 1, in <module>
    assert 1 <= i
AssertionError

It is already obvious that i must be >= 1. So I would reject the patch.

> And isn't this too much defensive programming?

Whether stdlib python code should have asserts is a more interesting question. I will ask on pydev.
msg203002 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-11-16 01:51
Looking further, the current code has a message object, the month that fails the test and your patch removes that in adding the redundant message. I also see that your change would make the first assert match the next 2. But I would rather change the next two.

Sequences like
_DI4Y   = _days_before_year(5) 
# A 4-year cycle has an extra leap day over what we'd get from pasting
# together 4 single years.
assert _DI4Y == 4 * 365 + 1

are bizarre. The constant should be directly set to 4*365 + 1 and then _days_before_year(5) == _DI4Y tested in test_datetime.
msg206125 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2013-12-13 17:22
I am going to reject this.  Assert failures should never be seen by users and for a developer "assert 1 <= month <= 12" is as clear as "month must be in 1..12."
msg206127 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2013-12-13 17:30
@terry - datetime.py was originally written as a prototype for the C code and many seemingly unpythonic constructs therein are motivated by the desire to ease the translation to C.

I would not mind simplifying _DI4Y calculation as you suggest, but please check how it is done in C.  I would like to keep the two implementation as similar as possible.

Please open a separate issue if you would like to have this done.
msg206147 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-12-13 21:25
Thank you for the explanation. If the style comment is not in the file already, you might add it whenever you next edit the file for substantive purposes (a real bug or feature change). Ditto for _DI4Y.
History
Date User Action Args
2013-12-13 21:25:03terry.reedysetmessages: + msg206147
2013-12-13 17:30:26belopolskysetmessages: + msg206127
2013-12-13 17:22:48belopolskysetstatus: open -> closed

nosy: + belopolsky
messages: + msg206125

resolution: wont fix
stage: resolved
2013-11-16 01:51:44terry.reedysetmessages: + msg203002
2013-11-16 00:57:48terry.reedysetversions: + Python 3.4, - Python 2.7, Python 3.3
2013-11-16 00:57:30terry.reedysetnosy: + terry.reedy
messages: + msg202998
2013-11-12 17:40:59thatiparthycreate