On 2 Nov 2010 at 17:05, Antoine Pitrou wrote:

> * the command-line option for the SSLContext won't work, since a
> context is a custom object, not a string; I would rework this part
> anyway, since I don't think separate options for the "SSL host" are
> useful (I'd rather add a SSL-enabling flag; also, STARTTLS can be
> queried from the capabilities)

I'm not sure I understand which part you're talking about here....the 
tests don't take a context for anything that I can see, and the 
mechanism I use in nntplib (a context as a named argument) is the 
same one used in smtplib and poplib for <prot>_SSL. 

(if you can suggest a method-of-use that breaks it, that may enlighten
me better) 

> * on a stylistic note, there are a couple of places where you use tabs
> for indentation; also, comments should have a space after the '#' ('#
> xxx' and not '#xxx')

Meh. My editor wasn't set up properly when I started. I thought I'd 
eliminated all the tabs. Guess not. There's probably a way to grep 
for lines that have tabs in them, I'll fix it.

> * not moving methods around (such as getwelcome) would make it easier
> to review the real changes

I'll see what I can do. Some amount of that was necessary to fit 
STARTTLS in. (because init assumed you wanted to log in right away 
and never again) 

> * in test_starttls, I would clearly report that the server doesn't
> support STARTTLS, e.g.:

Can do. I didn't know about self.skip; I don't really understand the 
testing framework, I was just working by comparison to the other 
tests.

> * starttls() should probably test the `tls_on` attribute first and
> raise a ValueError if True (as you point out, a client mustn't attempt
> to start a new TLS session if one is already active).

...I actually meant to do exactly that, but forgot to put the check 
in. Thank you. 

--

Andrew.