Message120285
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. |
|
Date |
User |
Action |
Args |
2010-11-02 23:18:41 | ajvant | set | recipients:
+ ajvant, janssen, pitrou, giampaolo.rodola, christian.heimes, chasonr, brian.curtin, jelie |
2010-11-02 23:18:38 | ajvant | link | issue1926 messages |
2010-11-02 23:18:37 | ajvant | create | |
|