Author eric.smith
Recipients George.Dhoore, eric.smith, giampaolo.rodola, pitrou
Date 2011-03-08.00:54:29
SpamBayes Score 3.33067e-16
Marked as misclassified No
Message-id <1299545671.31.0.440976716219.issue11072@psf.upfronthosting.co.za>
In-reply-to
Content
I agree that the callback isn't needed, and it reflects the older coding style of much of the library (such as in retrlines). Instead, I'd make this a generator, yielding each of the dicts. (Actually in some ideal rewrite of ftplib, the whole callback "feature" used by retrlines would go away except as a compatibility feature, and internally everything would use generators instead of callbacks.)

Aren't you modifying the state on the server (via "OPTS MLST"), and then if you make a subsequent call without specifying "facts" you'll be using the value of "facts" from the previous call to MLSD? I don't think that's desirable, but short of calling "OPTS MLST" every time (possibly with the results of an initial FEAT) there's no way around it.

Just today I saw a filename with "; " in the name. I think you want to use .partition(' ') isolate the facts from the filename, and add a test with such a filename.

I don't like the "isdigit" test. Shouldn't this decision be left to the caller? What if a fact happens to look like an integer some of the time, but not always? Maybe "unique" is a hex string. I don't think you'd want it converted to an int on the occasions where it was all decimal digits, but a string otherwise. Also look at your "modify" facts. Those are not very useful as ints.

I don't think you should invent a pseudo-fact "name", in case the standard ever uses that. I'd prefer if you yielded tuples of (filename, fact-dict).

MLSD_DATA has newlines in it, so you get "\r\n\n" sequences in your test data.

If a fact=value string does not have an '=', you silently ignore it. I'd rather this raise an exception and not pass silently. It's not compliant. You should have a test for this.

It's possible for a value to have an '=' in it, so you don't want to use .split('='), you should use .partition('='). You should add such a fact to the test data.

Thanks for working on this. It will be a great addition to ftplib.
History
Date User Action Args
2011-03-08 00:54:31eric.smithsetrecipients: + eric.smith, pitrou, giampaolo.rodola, George.Dhoore
2011-03-08 00:54:31eric.smithsetmessageid: <1299545671.31.0.440976716219.issue11072@psf.upfronthosting.co.za>
2011-03-08 00:54:30eric.smithlinkissue11072 messages
2011-03-08 00:54:29eric.smithcreate