Hi Keith, Thanks for doing a thorough review. I've taken most of your suggestions but not all (see below).
An updated webrev is at: http://cr.opensolaris.org/~aalok/libtransfer-coderev/ A webrev that just contains the diffs from the original review (also contains bug fixes found during testing and changes for python2.6) is at: http://cr.opensolaris.org/~aalok/libtransfer-coderev-diffs/ On Tue, 27 Oct 2009, Keith Mitchell wrote: > General: > Single line docstrings should be on one line, including the opening/trailing > quotes > Multi-line docstrings should have a blank line before the closing quotes Done. > transfer_defs.py: > General: May benefit from an __all__ var. (Probably out of scope) Out of scope. > 116-117: Please align these with the open bracket on 115 if possible done. > transfer_mod.py: > General: Some of your docstrings got misaligned after switching from tabs to > spaces, it seems > 118/123: Just set "self.handle = None", no need to add an extra init > parameter > 171 & 179: I question the need for these methods, but I'll leave that for > another day... > 232: Change to "while True:" > 264: Seems to be an extra space after output done. > 258-279: (Non PEP8): We don't ever seem to close output, do we need to? I'll defer this to later. > 302,357: Just to be sure, do we have bugs to capture these TODO? Yep, captured in defect 7230. > 305-315: Trailing slashes not necessary, but can be left in if they improve > readability. Your call here. > 330,338,350: "if self.log_handler is not None:" > 452: "if patt is not None and patt.startswith('!'):" > 453,456, : negate should be a bool (set to True/False) > 473-476: use "is None" or "is not None" > 497: "if patt is not None:" > 535-570 or so: The indentation here indicates and long 'if' statements > indicates this might benefit from refactoring either now or later. > Regardless, convert to "is None" and "is not None" where appropriate. > 688: "is not None" > 740: "is not None" > > 749-753: "while True:" - better yet, use: > char = True: > while char: > char = pipe.stdout.read(1) > self.check_abort() > > 936: "is not None" > 994: "is not None" > 1038: "is not None" > 1128: "is not None" > 1225: "is not None" > 1272: "is not None" > 1424: I believe this line is redundant. The function that set_py_callback > references in libtransfer does a PyCallable_Check() on the arg passed in, > which will fail if None is given? All of the above changes have been incorporated. > 1416-1468: For Python2.6, please convert this from try/(try/except)/finally > to try/except/finally. > 1461: Bare except clauses are generally inappropriate in methods, however, > given the current usage scenarios for tm_perform_transfer, this is fine. I'm going to defer this for later as it's risky. Alok
