Repository navigation
calendar.isleap() not checking parameter type #54282
Description
| BPO | 10073 |
|---|---|
| Nosy | @birkenfeld, @terryjreedy, @abalkin, @merwok, @bitdancer |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/abalkin'
closed_at = <Date 2010-10-18.18:25:02.203>
created_at = <Date 2010-10-12.13:47:55.046>
labels = ['type-feature', 'library']
title = 'calendar.isleap() not checking parameter type'
updated_at = <Date 2010-10-19.18:39:18.230>
user = 'https://bugs.python.org/shadikka'bugs.python.org fields:
activity = <Date 2010-10-19.18:39:18.230>
actor = 'eric.araujo'
assignee = 'belopolsky'
closed = True
closed_date = <Date 2010-10-18.18:25:02.203>
closer = 'belopolsky'
components = ['Library (Lib)']
creation = <Date 2010-10-12.13:47:55.046>
creator = 'shadikka'
dependencies = []
files = ['19285']
hgrepos = []
issue_num = 10073
keywords = ['patch']
message_count = 30.0
messages = ['118424', '118426', '118430', '118458', '118461', '118464', '118467', '118827', '118829', '118830', '118903', '118904', '118973', '119036', '119048', '119052', '119055', '119056', '119060', '119082', '119093', '119119', '119144', '119145', '119146', '119148', '119149', '119151', '119152', '119156']
nosy_count = 7.0
nosy_names = ['georg.brandl', 'terry.reedy', 'belopolsky', 'eric.araujo', 'Retro', 'r.david.murray', 'shadikka']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10073'
versions = ['Python 3.2']
Activity
shadikka commentedon Oct 12, 2010
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 "<stdin>", line 1, in <module> 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 formattingA quick peek at the SVN shows that the problem still exists.
bitdancer commentedon Oct 12, 2010
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...)
abalkin commentedon Oct 12, 2010
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".
abalkin commentedon Oct 12, 2010
I would also like to note the following curiosity:
>>> calendar.isleap("%d") FalseI 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.
merwok commentedon Oct 12, 2010
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”).
shadikka commentedon Oct 12, 2010
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.
pjenvey commentedon Oct 12, 2010
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
birkenfeld commentedon Oct 15, 2010
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.
terryjreedy commentedon Oct 15, 2010
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.
birkenfeld commentedon Oct 15, 2010
You'll always find plenty "wrong" inputs for which a function doesn't
raise an exception. That's the downside of duck typing.
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 agree that
year & 3 == 0in this place is okay, and if it helpsavoid confusion, all the better.
7 remaining items
Retro commentedon Oct 18, 2010
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 <
[email protected]> wrote:
abalkin commentedon Oct 18, 2010
I don't think anything good will come out of this. Closing as "rejected."
Retro commentedon Oct 18, 2010
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.
birkenfeld commentedon Oct 18, 2010
Not only is this issue already closed, the patch is also wrong -- your change to the parentheses changes the semantics.
merwok commentedon Oct 19, 2010
On a closing note, I’ll let you enjoy the Zen of Python:
$ python -m this
Retro commentedon Oct 19, 2010
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 <[email protected]> wrote:
abalkin commentedon Oct 19, 2010
On Tue, Oct 19, 2010 at 12:34 PM, Boštjan Mejak <[email protected]> wrote:
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: bpo-4389, bpo-4648, bpo-4649, and bpo-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.
Retro commentedon Oct 19, 2010
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 <
[email protected]> wrote:
Retro commentedon Oct 19, 2010
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.
abalkin commentedon Oct 19, 2010
Committed the docstring patch in r85725.
Retro commentedon Oct 19, 2010
Thank you for applying my patch. Glad to help. Please apply it to the trunk and other branches as well.
birkenfeld commentedon Oct 19, 2010
It will be merged in due time.
merwok commentedon Oct 19, 2010