[Patches] [ python-Patches-553277 ] Little ftplib bug
noreply@sourceforge.net
noreply@sourceforge.net
Wed, 08 May 2002 06:49:12 -0700
Patches item #553277, was opened at 2002-05-07 16:02
You can respond by visiting:
http://sourceforge.net/tracker/?func=detail&atid=305470&aid=553277&group_id=5470
Category: Library (Lib)
Group: Python 2.2.x
>Status: Open
Resolution: Accepted
Priority: 5
Submitted By: Geert Jansen (geertj)
Assigned to: Nobody/Anonymous (nobody)
Summary: Little ftplib bug
Initial Comment:
There seem to be a little bug in FTP.retrlines in the
module ftplib in the standard library. In this
function, the default value for the "callback"
parameter is None. The absence of a callback parameter
is tested in the body if the function with "if not
callback:".
This gives problems in real-life when you're using a
callable container class as the callback function.
When the container is empty (__len__ returns 0), the
test "if not callback:" is true and a default callback
is used instead of the user supplied one.
The fix is to change the test "if not callback" to "if
callback is None:". One-line patch attached.
Greetings,
Geert Jansen
----------------------------------------------------------------------
>Comment By: Martin v. Löwis (loewis)
Date: 2002-05-08 15:49
Message:
Logged In: YES
user_id=21627
How about we go back to the original patch, testing for None
explicitly?
I understand that the problem of a "false" value being
ignored was real - Geert, can you confirm?
I'd rather accept that calling an uncallable false value
would now cause an exception; if there is concern that this
may break code, that could be mentioned in Misc/NEWS.
Anyway, reopening.
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2002-05-08 15:20
Message:
Logged In: YES
user_id=6380
I think the "if callable():" test is wrong. Now if someone
passes an unusable callback it will be ignored rather than
raising an exception. I don't find that an improvement (the
bug in the code will go undetected longer). I don't think
that that the worry about people passing some other form of
"false" is warranted.
----------------------------------------------------------------------
Comment By: Martin v. Löwis (loewis)
Date: 2002-05-08 10:57
Message:
Logged In: YES
user_id=21627
Thanks for the patch; committed as ftplib.py 1.67, ACKS 1.172.
----------------------------------------------------------------------
Comment By: Raymond Hettinger (rhettinger)
Date: 2002-05-08 10:26
Message:
Logged In: YES
user_id=80475
That was quick!
You have my sign-off.
Will recommend to a developer for a commit.
----------------------------------------------------------------------
Comment By: Geert Jansen (geertj)
Date: 2002-05-08 10:02
Message:
Logged In: YES
user_id=537938
Hi!
Thanks for your comments. I uploaded a new version of the
patch, relative to version 1.66 of ftplib.py, that uses the
callable(callback) variant. I tested the patch and verified
that it works as expected.
----------------------------------------------------------------------
Comment By: Raymond Hettinger (rhettinger)
Date: 2002-05-08 07:14
Message:
Logged In: YES
user_id=80475
One other thought. Consider changing the line to:
if not callable(callback): callback = print_line
This would eliminate the code breakage I outlined in the
previous comment.
----------------------------------------------------------------------
Comment By: Raymond Hettinger (rhettinger)
Date: 2002-05-08 07:01
Message:
Logged In: YES
user_id=80475
Please update the patch to diff from the current version of
ftplib.py (revision 1.66).
I agree that 'if callback is None' more accurately captures
the intent of checking that a callback function was not
specified. It is also stylistically consistent with the 'is
None' test in the definition of the ntransfercmd function
and in other None checks elsewhere in the module.
Also, I cross-checked the docs and found no conflict. I
tested retrlines with the patch in-place and found no ill-
effects. The problem being solved is a bit obscure in that
the callback would need to be a user class defining both
__len__ and __call__.
The only code breakage I could think of would be a case
where someone relied on the existing implementation, though
the documentation makes no promises, and had their calling
code default to a callback value equivalent to False but
not identical to None:
def getfile(ftpobj, filename, callback=[]):
'This code works before the patch, but not after'
ftpobj.retrlines('retr '+filename, callback)
----------------------------------------------------------------------
You can respond by visiting:
http://sourceforge.net/tracker/?func=detail&atid=305470&aid=553277&group_id=5470