[Patches] [ python-Patches-553277 ] Little ftplib bug

[noreply@sourceforge.net]

Patches item #553277, was opened at 2002-05-07 10: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: Closed
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: Guido van Rossum (gvanrossum)
Date: 2002-05-08 09: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 04: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 04: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 04: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 01: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 01: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