Author pitrou
Recipients Dmitry.Jemerov, giampaolo.rodola, ncoghlan, pitrou, r.david.murray
Date 2010-09-20.18:20:32
SpamBayes Score 2.4647e-14
Marked as misclassified No
Message-id <1285006829.3228.11.camel@localhost.localdomain>
In-reply-to <1284948286.97.0.989901474065.issue9360@psf.upfronthosting.co.za>
Content
> Gah.  I reviewed patch4, didn't noticed today's update :(

Oops, I'm sorry for that. It turns out most of your comments are useful anyway.

> Why is it a good thing to make file a keyword only parameter?

I was thinking that if we want to add other function parameters, it will
be possible to do so while keeping the (mnemonically useful) convention
that the file parameter is last.

You are right that testing that parameter is still on the TODO list...

> On line 193 you index fmt, and *then* you check the length.  When the
> number of tokens is too long, an IndexError is raised.

Yes, this is fixed in patch5 :)

> Could the 'date' field in the xover headers also be a DateTime rather
> than a string?  And :bytes and :lines be ints?  Or is that being to
> DWIMish?

We could indeed parse the xover/over fields, but the issue is that there
can be extra fields which therefore won't get parsed (or not all of them
if we decide to recognize some e.g. "Xref").

Also, people could want to get at the non-parsed representation
(especially for the date field), which means we would need two APIs with
two different levels of abstraction.

> If the date field isn't returned as a datetime, though, should there
> be a helper method for converting it, or should we just assume that
> email.utils mktime_tz and parsedate_tz?

parsedate or parsedate_tz is the right thing to use indeed (dates follow
the same format as in e-mails). As for the from and subject fields, the
provided decode_header() is very recommended (as the __main__ block
shows).

> Am I correct that the purpose of _NNTPBase is to make testing easier?

Yes.

> There seem to be only three lines of code in common between the file
> and the non-file case in _getlongresp.  I think it would be clearer to
> make them two different routines, or at least move the three common
> lines to the top and then do an if test on file is None (that is, put
> the simpler, non-file case first).

I'll take a look.

> My little nttp client script doesn't really test very much of the
> nttplib interface, nor is it very complex.  The change to xover
> considerably simplified that part of my script, so I very much like
> that change.  I was also able to drop my implementation of
> decode_header.  So overall this patch is significant improvement, IMO.

Thank you :)
History
Date User Action Args
2010-09-20 18:20:35pitrousetrecipients: + pitrou, ncoghlan, giampaolo.rodola, r.david.murray, Dmitry.Jemerov
2010-09-20 18:20:34pitroulinkissue9360 messages
2010-09-20 18:20:32pitroucreate