Hi Alok,
Just a few questions on the updated webrev, in transfer_mod.py:
144: Why did you change from using "super()"? I'm just curious, as from
what I've seen, both syntaxes are equally "correct" when considering
single inheritance.
472: The first part of this 'if' statement should stay as it was
('old_cprefix != cp.chdir_prefix').
560, 562: Use "negate" and "not negate" here instead of "negate == True"
/ "negate == False"
590: If this fits on the previous line, it would be better off there.
Everything else looks good. Also, your assessment of which comments to
defer/skip works for me.
Thanks,
Keith
Alok Aggarwal wrote:
> 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
