Keith Mitchell wrote:
> Hi Alok,
>
> Per usual, if my changes creep beyond PEP8 scope, just say so.
>
> Thanks,
> Keith
>
> 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
>
> transfer_defs.py:
> General: May benefit from an __all__ var. (Probably out of scope)
> 116-117: Please align these with the open bracket on 115 if possible
>
> 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
> 258-279: (Non PEP8): We don't ever seem to close output, do we need to?
> 302,357: Just to be sure, do we have bugs to capture these TODO?
> 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?
> 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.
>
> Alok Aggarwal wrote:
>> Please review my changes to make libtransfer PEP8
>> compliant.
>>
>> http://cr.opensolaris.org/~aalok/libtransfer-pep8/
>>
>> The changes are currently being tested but they get
>> a 10 from pylint.
>>
>> Thanks,
>> Alok
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Keith,
I see the value in making the code consistent conform to standards but
we need a way to identify what doesn't conform without relying on manual
code reviews.
I think we should only do what pylint reports using the script in our
gate - the one Jean pushed recently. Issues not picked up by:
pylint --rcfile=./usr/src/tools/env/install.pylintrc <file>.py
should not be required.
Maybe we need to update the script to pick up more.
Joe
