Message120239
Andrew, thank you for posting a patch.
I have a couple of comments:
* 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)
* 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')
* not moving methods around (such as getwelcome) would make it easier to review the real changes
* in test_starttls, I would clearly report that the server doesn't support STARTTLS, e.g.:
except nntplib.NNTPPermanentError:
self.skip("STARTTLS not supported by server")
* 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).
All in all, it looks good though. |
|
Date |
User |
Action |
Args |
2010-11-02 17:05:40 | pitrou | set | recipients:
+ pitrou, janssen, giampaolo.rodola, christian.heimes, chasonr, brian.curtin, ajvant, jelie |
2010-11-02 17:05:39 | pitrou | set | messageid: <1288717539.98.0.790128608117.issue1926@psf.upfronthosting.co.za> |
2010-11-02 17:05:37 | pitrou | link | issue1926 messages |
2010-11-02 17:05:35 | pitrou | create | |
|