| msg92287 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-09-05 18:00 |
| FPT class doesn't support the rest parameter in storbinary method. |
|
|
| msg92290 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-09-05 18:42 |
| I attached a patch. |
|
|
| msg92503 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-09-11 02:43 |
| I'm changing the title to something clearer (I hope). The patch I submitted adds the retr optional parameter to the storbinary method. |
|
|
| msg92970 - (view) |
Author: Facundo Batista (facundobatista) *  |
Date: 2009-09-22 02:20 |
| I like this. I'd love to see a test of this, though. Pablo, do you think you could came up with a test? Thanks! |
|
|
| msg93469 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-10-03 01:28 |
| I attached some tests. |
|
|
| msg94286 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-10-20 15:20 |
| According to the RFC, the argument to REST can be any string of printable characters. However, does it happen for clients to put non-digits in there? (that is, are there any implementations where the argument to REST is something else than the byte offset from the start of file?) |
|
|
| msg94292 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-10-20 16:50 |
| I think that a non-digit REST argument is an error. But I've tested some ftp servers and they don't return an error code, they just set REST to 0 and return some OK code. Finally, ftplib doesn't check that, so if someone accidentally passes a non-digit argument it could be a difficult bug to spot. |
|
|
| msg94294 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2009-10-20 17:58 |
| The patch looks good to me. Only two little remarks: 1: I'd create two new separate tests rather than appending them to existent ones. 2: > self.client.retrbinary('retr', received.append, rest=str(rest)) str() should be useless here. > According to the RFC, the argument to REST can be any string of > printable characters. However, does it happen for clients to put > non-digits in there? It shouldn't happen but in any case I woulnd't want ftplib to check for such a kind of thing. Deciding whether the REST argument is invalid is up to the server, in which case it will send a 4xx/5xx error response. > But I've tested some ftp servers and they don't return an > error code, they just set REST to 0 and return some OK code. IMHO a bad design choice. |
|
|
| msg94301 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-10-20 21:23 |
| > I think that a non-digit REST argument is an error. But I've tested some > ftp servers and they don't return an error code, they just set REST to 0 > and return some OK code. Finally, ftplib doesn't check that, so if > someone accidentally passes a non-digit argument it could be a difficult > bug to spot. What I mean is that integer arguments should be accepted as well, since that's what is logical to produce. |
|
|
| msg95100 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-11-10 02:19 |
| Here is a new patch. It works with both ints and strings. I'm working on a patch for py3k. |
|
|
| msg95754 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-11-26 22:58 |
| Now that FTP_TLS has been integrated into the trunk, the patch should be augmented in order to also cover that class. Right now a test is failing: ====================================================================== ERROR: test_storbinary_rest (test.test_ftplib.TestTLS_FTPClassMixin) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line 485, in test_storbinary_rest self.client.storbinary('stor', f, rest=r) TypeError: storbinary() got an unexpected keyword argument 'rest' Apart from that, the patch looks ok. |
|
|
| msg95756 - (view) |
Author: Pablo Mouzo (pablomouzo) |
Date: 2009-11-26 23:43 |
| Thanks Antoine, I'm attaching both patches. Now the test isn't failing. |
|
|
| msg95765 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-11-27 13:25 |
| The patch has been committed in r76546 and r76547. Thank you for your contribution! |
|
|