Issue 10073: calendar.isleap() not checking parameter type (original) (raw)
Created on 2010-10-12 13:47 by shadikka, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (30)
Author: (shadikka)
Date: 2010-10-12 13:47
calendar.isleap() doesn't enforce any types, leading to string formatting errors when a string is passed to it:
calendar.isleap("2011") Traceback (most recent call last): File "", line 1, in File "/Library/Frameworks/Python.framework/Versions/3.1/lib/python3.1/calendar.py", line 99, in isleap return year % 4 == 0 and (year % 100 != 0 or year % 400 == 0) TypeError: not all arguments converted during string formatting
A quick peek at the SVN shows that the problem still exists.
Author: R. David Murray (r.david.murray) * 
Date: 2010-10-12 14:49
In Python we often don't type check, we just let errors happen. It is true that it would make the problem clearer to do a type check and issue a specific error message, but I don't know if it is worth it. (The error would already have been clear if it weren't for the fact that % is the string formatting operator...)
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-12 16:02
I agree with David on this one. In addition to the arguments already stated, the string value of an exception is not part of specification. The exception is still TypeError even if the message may be confusing. I also find that once read in the context of the backtrace, the error is clear enough.
I am changing this to an RFE for now, but I think this should be closed as "won't fix".
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-12 18:53
I would also like to note the following curiosity:
calendar.isleap("%d") False
I still don't think there is anything worth fixing here, but we can consider replacing year % 4 == 0 predicate with year & 3 == 0 which will change the error raised by isleap("2004") from
TypeError: not all arguments converted during string formatting
to
TypeError: unsupported operand type(s) for &: 'str' and 'int'
I am not sure if the later is much clearer than the former once you consider obfuscated year & 3 == 0 showing up in the backtrace compared to a clear year % 4 == 0 test.
Author: Éric Araujo (eric.araujo) *
Date: 2010-10-12 19:13
I’d be in favor of type checking here, but I know I’m in the minority. I’m very much not in favor of making code less readable (“& 3”).
Author: (shadikka)
Date: 2010-10-12 19:47
To clarify my original point, I'm for making an explicit error message for that, definitely not for any silent failure or automatic string conversion.
Author: Philip Jenvey (pjenvey) *
Date: 2010-10-12 20:29
Another option is to wrap the operations in a try/except. When a TypeError is raised have it throw a new TypeError with an improved error message and the original chained to it
Author: Georg Brandl (georg.brandl) *
Date: 2010-10-15 19:52
I don't see the point. If you file one such bug per day, you won't be finished in a year -- there's no reason to start adding typechecking in this function.
Author: Terry J. Reedy (terry.reedy) *
Date: 2010-10-15 20:45
To me, Alexander's example
calendar.isleap("%d") False is a buggy result. So I would reclassify the issue.
The rationale for not checking input types is that bad types result in an error, but that does not happen here due to a design decision that some consider clever and some buggy, and some both. (Guido himself would like to deprecate it.)
I am in favor of the 'year & 3 == 0' fix so that any input string (and indeed, anything that does not implement both % and & operators) raises an error. Intermediate to expert programmers should know or learn about bit masking. Add a comment to explain the reason -- and a test case.
Author: Georg Brandl (georg.brandl) *
Date: 2010-10-15 20:51
To me, Alexander's example
calendar.isleap("%d") False is a buggy result. So I would reclassify the issue.
You'll always find plenty "wrong" inputs for which a function doesn't raise an exception. That's the downside of duck typing.
The rationale for not checking input types is that bad types result in an error, but that does not happen here due to a design decision that some consider clever and some buggy, and some both. (Guido himself would like to deprecate it.)
You're talking of % string formatting? Well, that's just one operation where duck typing applies. There may be a bit less chance of namespace collisions when calling methods, but it's the same issue at heart.
I am in favor of the 'year & 3 == 0' fix so that any input string (and indeed, anything that does not implement both % and & operators) raises an error. Intermediate to expert programmers should know or learn about bit masking. Add a comment to explain the reason -- and a test case.
I agree that year & 3 == 0 in this place is okay, and if it helps
avoid confusion, all the better.
Author: Boštjan Mejak (Retro)
Date: 2010-10-16 22:15
Please leave this function as is because it works just fine.
calendar.isleap(2011) False
The argument for isleap() must not be in quotes. That's all.
Author: Boštjan Mejak (Retro)
Date: 2010-10-16 22:21
I would just add an exception to the isleap() function.
def isleap(year): """Return 1 for leap years, 0 for non-leap years.""" try: return year % 4 == 0 and (year % 100 != 0 or year % 400 == 0) except TypeError: # somehow inform the user that the argument 'year' must be an integer
Author: Boštjan Mejak (Retro)
Date: 2010-10-17 19:54
Let me fix this function a little bit...
def isleap(year): """Return True for leap years, False for non-leap years.""" if year == 0: raise ValueError('year 0 does not exist') return (year % 4 == 0) and (year % 100 != 0) or (year % 400 == 0)
This function, however, does not mind if you give it a negative number. But I think we can leave this option to mean B.C. (Before Christ), so calendar.isleap(-240) would mean 240 B.C., which was a leap year.
About the if year == 0 check... Well, read Wikipedia's article http://en.wikipedia.org/wiki/0_(year) which clearly states that "Year zero does not exist in the widely used Gregorian calendar or in its predecessor, the Julian calendar." So we raise a ValueError if this value is used in the calendar.isleap() function.
I have uploaded a patch that fixes this function. Please apply it to the trunk and also to the maintenance brances.
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-18 16:38
On Sun, Oct 17, 2010 at 3:54 PM, Boštjan Mejak <report@bugs.python.org> wrote: ..
About the if year == 0 check... Well, read Wikipedia's article http://en.wikipedia.org/wiki/0_(year) which clearly states that "Year zero does not exist in the widely used Gregorian calendar or in its predecessor, the Julian calendar." So we raise a ValueError if this value is used in the calendar.isleap() function.
Well, Wikipedia is not the most authoritative source, but if you go the right article there, you will find that
""" Mathematically, it is more convenient to include a year zero and represent earlier years as negative, for the specific purpose of facilitating the calculation of the number of years between a negative (BC) year and a positive (AD) year. This is the convention used in astronomical year numbering and in the international standard date system, ISO 8601. In these systems, the year 0 is a leap year.[2] """
Python documentation states that calendar module implements 'the “proleptic Gregorian” calendar in Dershowitz and Reingold’s book “Calendrical Calculations”'. See http://docs.python.org/dev/py3k/library/calendar.html
I don't have Dershowitz and Reingold’s book to check, but ISO/FDIS 8601:2000(E) standard contains a note that states: "In the prolaptic [sic] Gregorian calendar the calendar year [0000] is a leap year."
In any case, I think there are more important issues with the calendar module than perceived bugs in isleap() function. See for example issue 10087.
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-18 17:59
The most pedantic implementation of calendar.isleap() would be
from datetime import date, timedelta def isleap(year): return date(year, 3, 1) - date(year, 2, 1) == timedelta(29)
Since python calendar only supports years in the range [1, 9999], the above will properly raise ValueError: year is out of range on negative or zero year. This also guarantees that calendar module's notion of leap year is the same as that of datetime module.
If this is found to be a worthwhile change, I would also rewrite monthrange as
def monthrange(year, month): first = date(year, month, 1) if month == 12: ndays = 31 else: ndays = (date(year, month + 1, 1) - first).days return first.weekday(), ndays
Author: Boštjan Mejak (Retro)
Date: 2010-10-18 18:12
Also, your "pedantic" version of isleap() is not pedantic at all.
return date(year, 3, 1) - date(year, 2, 1) == timedelta(29) does not seem readable at all. Readability counts!
return date(year, 3, 1) is not understandable. What are the arguments 3 and 1 in the date() function for?
On Mon, Oct 18, 2010 at 8:06 PM, Boštjan Mejak <bostjan.mejak@gmail.com>wrote:
else:--> ndays = (date(year, month + 1, 1) - first).days return first.weekday(), ndays
Oh my God! The line with a pointer is so ugly!
On Mon, Oct 18, 2010 at 7:59 PM, Alexander Belopolsky < report@bugs.python.org> wrote:
Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:
The most pedantic implementation of calendar.isleap() would be
from datetime import date, timedelta def isleap(year): return date(year, 3, 1) - date(year, 2, 1) == timedelta(29)
Since python calendar only supports years in the range [1, 9999], the above will properly raise ValueError: year is out of range on negative or zero year. This also guarantees that calendar module's notion of leap year is the same as that of datetime module.
If this is found to be a worthwhile change, I would also rewrite monthrange as
def monthrange(year, month): first = date(year, month, 1) if month == 12: ndays = 31 else: ndays = (date(year, month + 1, 1) - first).days return first.weekday(), ndays
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue10073>
Author: Georg Brandl (georg.brandl) *
Date: 2010-10-18 18:17
return date(year, 3, 1) is not understandable. What are the arguments 3 and 1 in the date() function for?
Boštjan, we appreciate your concern for the programming style of the Python standard library. However, your question shows that you should probably spend a bit more time reading documentation than "fixing" code.
Author: Boštjan Mejak (Retro)
Date: 2010-10-18 18:17
else: --> ndays = (date(year, month + 1, 1) - first).days return first.weekday(), ndays
Oh my God! The line with a pointer is so ugly!
On Mon, Oct 18, 2010 at 7:59 PM, Alexander Belopolsky < report@bugs.python.org> wrote:
Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:
The most pedantic implementation of calendar.isleap() would be
from datetime import date, timedelta def isleap(year): return date(year, 3, 1) - date(year, 2, 1) == timedelta(29)
Since python calendar only supports years in the range [1, 9999], the above will properly raise ValueError: year is out of range on negative or zero year. This also guarantees that calendar module's notion of leap year is the same as that of datetime module.
If this is found to be a worthwhile change, I would also rewrite monthrange as
def monthrange(year, month): first = date(year, month, 1) if month == 12: ndays = 31 else: ndays = (date(year, month + 1, 1) - first).days return first.weekday(), ndays
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue10073>
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-18 18:25
I don't think anything good will come out of this. Closing as "rejected."
Author: Boštjan Mejak (Retro)
Date: 2010-10-18 21:16
I have uploaded a new patch. It removes the if year == 0 nonsense. Check it out. I hope you like it now.
When you were organizing the Standard Library, you didn't fix flaws in the calendar module. What I have in mind is, for example, the isleap() function which should be defined as is_leap(). But now it's all too late for that fixes.
Please apply my patch for calendar.isleap() to the trunk. Thanks.
Author: Georg Brandl (georg.brandl) *
Date: 2010-10-18 23:33
Not only is this issue already closed, the patch is also wrong -- your change to the parentheses changes the semantics.
Author: Éric Araujo (eric.araujo) *
Date: 2010-10-19 04:45
for example, the isleap() function which should be defined as is_leap() Who says that? Not PEP 8: “Function names should be lowercase, with words separated by underscores as necessary to improve readability” (emphasis mine). isleap is perfectly readable.
But now it's all too late for that fixes. You’ll find that Python has a balance between readability/ease of use and pragmatism. In some cases, changes can’t be made, but it’s best to accept it and move on to the next issue. Working on feature requests instead of style issues is great.
On a closing note, I’ll let you enjoy the Zen of Python: $ python -m this
Author: Boštjan Mejak (Retro)
Date: 2010-10-19 16:34
I have read the Zen of PYthon before. I have also written a typo report. But got rejected. No one listens to my ideas. So why do you have a bug tracker anyway?
On Tue, Oct 19, 2010 at 6:46 AM, Éric Araujo <report@bugs.python.org> wrote:
Éric Araujo <merwok@netwok.org> added the comment:
for example, the isleap() function which should be defined as is_leap() Who says that? Not PEP 8: “Function names should be lowercase, with words separated by underscores as necessary to improve readability” (emphasis mine). isleap is perfectly readable.
But now it's all too late for that fixes. You’ll find that Python has a balance between readability/ease of use and pragmatism. In some cases, changes can’t be made, but it’s best to accept it and move on to the next issue. Working on feature requests instead of style issues is great.
On a closing note, I’ll let you enjoy the Zen of Python: $ python -m this
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue10073>
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-19 17:02
On Tue, Oct 19, 2010 at 12:34 PM, Boštjan Mejak <report@bugs.python.org> wrote:
I have also written a typo report. But got rejected. No one listens to my ideas. So why do you have a bug tracker anyway?
Bug tracker is not the place to discuss ideas. We have python-ideas mailing list for that.
I searched the tracker and found four documentation issues that you submitted in the past: #4389, #4648, #4649, and #6475. In each case, your issue received attention from the most senior python developers. In each case the issue was closed with a detailed comment explaining the reasons for rejection.
I think if you search the tracker for issues with "accepted" or "fixed" resolution, you will understand why we have the bug tracker. Reading successful reports may also help you to submit better reports in the future.
Author: Boštjan Mejak (Retro)
Date: 2010-10-19 17:27
your change to the parentheses changes the semantics. (Georg Brandl)
How does adding those parens change semantics? The behaviour of the function isleap() is the same. I have throughtly tested this function and found no semantic errors in it. So the parens don't matter for behaviour, only for clarity they matter. But since you decided not to fix this, I won't push the envelope. At least fix the docstrings to match the return value. Please fix the docstrings to """Return True for leap years, False for non-leap years."""
On Tue, Oct 19, 2010 at 7:02 PM, Alexander Belopolsky < report@bugs.python.org> wrote:
Alexander Belopolsky <belopolsky@users.sourceforge.net> added the comment:
On Tue, Oct 19, 2010 at 12:34 PM, Boštjan Mejak <report@bugs.python.org> wrote:
I have also written a typo report. But got rejected. No one listens to my ideas. So why do you have a bug tracker anyway?
Bug tracker is not the place to discuss ideas. We have python-ideas mailing list for that.
I searched the tracker and found four documentation issues that you submitted in the past: #4389, #4648, #4649, and #6475. In each case, your issue received attention from the most senior python developers. In each case the issue was closed with a detailed comment explaining the reasons for rejection.
I think if you search the tracker for issues with "accepted" or "fixed" resolution, you will understand why we have the bug tracker. Reading successful reports may also help you to submit better reports in the future.
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue10073>
Author: Boštjan Mejak (Retro)
Date: 2010-10-19 17:37
I am applying a patch that only fixes the docstrings to match the return value of calendar.isleap(). The return value of isleap() is not 1 or 0 -- it is True or False. Please apply the patch. Thank you.
Author: Alexander Belopolsky (belopolsky) *
Date: 2010-10-19 17:44
Committed the docstring patch in r85725.
Author: Boštjan Mejak (Retro)
Date: 2010-10-19 18:16
Thank you for applying my patch. Glad to help. Please apply it to the trunk and other branches as well.
Author: Georg Brandl (georg.brandl) *
Date: 2010-10-19 18:19
It will be merged in due time.
Author: Éric Araujo (eric.araujo) *
Date: 2010-10-19 18:39
How does adding those parens change semantics? It changes boolean evaluation paths. See for example http://docs.python.org/py3k/reference/expressions#boolean-operations . If you have more questions, python-list is an open and friendly mailing list dedicated to answering them.
History
Date
User
Action
Args
2022-04-11 14:57:07
admin
set
github: 54282
2010-10-19 18:39:18
eric.araujo
set
messages: +
2010-10-19 18:19:11
georg.brandl
set
messages: +
2010-10-19 18:16:27
Retro
set
messages: +
2010-10-19 17:44:31
belopolsky
set
messages: +
2010-10-19 17:37:42
Retro
set
files: + isleap-docstrings.patch
messages: +
2010-10-19 17:33:05
Retro
set
files: - calendar-isleap.patch
2010-10-19 17:32:59
Retro
set
files: - unnamed
2010-10-19 17:27:36
Retro
set
files: + unnamed
messages: +
2010-10-19 17:02:27
belopolsky
set
messages: +
2010-10-19 16:40:51
belopolsky
set
files: - unnamed
2010-10-19 16:34:38
Retro
set
files: + unnamed
messages: +
2010-10-19 04:45:59
eric.araujo
set
messages: +
2010-10-18 23:33:30
georg.brandl
set
messages: +
2010-10-18 21:17:57
pjenvey
set
nosy: - pjenvey
2010-10-18 21:16:27
Retro
set
files: + calendar-isleap.patch
messages: +
2010-10-18 21:03:45
Retro
set
files: - calendar-isleap.patch
2010-10-18 18:25:27
belopolsky
set
files: - unnamed
2010-10-18 18:25:20
belopolsky
set
files: - unnamed
2010-10-18 18:25:02
belopolsky
set
status: open -> closed
resolution: rejected
messages: +
2010-10-18 18:17:09
Retro
set
files: + unnamed
messages: +
2010-10-18 18:17:06
georg.brandl
set
nosy: + georg.brandl
messages: +
2010-10-18 18:12:47
Retro
set
files: + unnamed
messages: +
2010-10-18 18:10:34
georg.brandl
set
nosy: - georg.brandl
2010-10-18 17:59:25
belopolsky
set
messages: +
2010-10-18 16:38:52
belopolsky
set
messages: +
2010-10-17 19:54:23
Retro
set
files: + calendar-isleap.patch
keywords: + patch
messages: +
2010-10-16 22:21:36
Retro
set
messages: +
2010-10-16 22:15:13
Retro
set
nosy: + Retro
messages: +
2010-10-15 20:51:15
georg.brandl
set
messages: +
2010-10-15 20:45:32
terry.reedy
set
nosy: + terry.reedy
messages: +
2010-10-15 19:52:14
georg.brandl
set
nosy: + georg.brandl
messages: +
2010-10-12 20:29:06
pjenvey
set
nosy: + pjenvey
messages: +
2010-10-12 19:47:32
shadikka
set
messages: +
2010-10-12 19:13:09
eric.araujo
set
messages: +
2010-10-12 18:53:52
belopolsky
set
messages: +
2010-10-12 17:03:27
eric.araujo
set
nosy: + eric.araujo
2010-10-12 16:02:04
belopolsky
set
assignee: belopolsky
type: behavior -> enhancement
messages: +
versions: - Python 3.1, Python 2.7
2010-10-12 14:49:24
r.david.murray
set
nosy: + r.david.murray
messages: +
versions: + Python 3.1, Python 3.2, - Python 3.3
2010-10-12 13:55:47
pitrou
set
nosy: + belopolsky
2010-10-12 13:48:07
shadikka
set
type: behavior
2010-10-12 13:47:55
shadikka
create