Joseph J VLcek wrote:
> 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.
We probably do. That said, a certain amount of manual verification will
be needed, I think, to catch all PEP8 'bullet points', as some of them
aren't readily scriptable. For example, knowing when to use "if x", "if
x is not None", and "if x == y" depends on what x and y are, and what
the code is evaluating. Also, I don't know Pylint well enough to know if
the stuff that is scriptable, but is getting missed (e.g., the docstring
notes) are available there, or if we would need a second script for
checking those. Remember that Pylint != PEP8, though there is some overlap.
- Keith
>
> Joe
