Code review comment for lp:~doxxx/bzr/quiet-serve

Revision history for this message
Vincent Ladeuil (vila) wrote :

Really tweaks:

+ class _TestBzrServeBase(TestCaseWithTransport):

We never use leading '_' for test classes, even if that may sound reasonable here.
I.e. I understand the intent, but let's not surprise future readers by starting
a new coding rule without good reason.

+ try:
+ self.run_bzr(['serve'] + list(serve_args))
+ except KeyboardInterrupt as e:
+ return e.args

1) I doubt python2.4 will accept 'as'.

2) While it's understandable in this patch context that e.args is really stdout, stderr, it will quickly become obscure. A comment will address that.

3) If the exception is raised, None will be returned instead of the (stdout, stderr) tuple. Again, today, the only that cares will pass, but if someone try to reuse the run_bzr_serve_then_func method, he will have a hard time. Please return (None, None) at least.

review: Needs Fixing

« Back to merge proposal