Keith,
Thanks for all the comments. I will update the code to match most of
your suggestions.
Some of them won't be possible, like for example function name changing.
I agree the current function names are bad, but changing them is out of
the scope of this task.
Cheers,
Luis
Keith Mitchell wrote:
> Hi,
>
> Comments below. Standard disclaimer: Please feel free to ignore any
> changes that are out-of-scope of PEP8/pylint/2.4->2.6, or would overly
> delay the integration of 2.4->2.6 changes.
>
> Thanks,
> Keith
>
> General: A lot of the variable names are short, abbreviated and
> confusing. If possible and reasonable, can we change, e.g., "be" to
> "boot_env", and vars such as 'at' on line 111 to "attrib" (or better,
> "attribute")
>
> General: If this is not too disruptive, please update function names
> to be lowercase with underscores, as per PEP8.
>
>
> 42: Capitalize class name to "ListBootEnvironment"
>
> 87: Should be "if 'orig_be_name' in be:"
> 92: "if be_name is not None..."
> 98: "if att in be and att in self.lattrs:"
> Please check for other instances of using "dict.has_key(val)" and
> replace with "val in dict" as appropriate (also in messages.py)
>
> 113: "if isinstance(self, SnapshotList) and att == 'snap_name' and att
> in be and '/' in be[att]:"
> 130: "if isinstance(self, BEList):"
>
> 144-145: Please add parentheses for readability, and try to break
> around the "and" clauses if possible.
>
> 260-261: Use 'isinstance(...)'
>
> beadm.py:
> Typo: Misspelled "snapshot" in function signature
> "trgt_be_name_or_snapshot" (since this function is getting a
> namechange anyway, perhaps the name could be shortened to something
> more reasonable also...)
>
> 192, 270,: "if '@' in be.trgt_be_name_or_snapshot[0]:"
> 201: "is not None"
> 202: " '@' in be.src_be_name_or_snapshot:"
>
> 316, 321, 324, 455: Alignment
>
> 322: Bare 'except' clause
>
> 356-357: Here, and in any other docstrings, renaming the function name
> is not needed.
>
> 453, 463, 555, 633, 698, 881, 924, 959, 1000, 1025: "is None"
>
> 527: "if not mountpoint.startswith('/'):"
>
> 721-725: Could be replaced with:
> for be_name in be.trgt_be_name_or_snapshot:
> msg.printMsg(msg.Msgs.BEADM_MSG_FREE_FORMT, be_name, -1)
>
> 742-760: Sidenote: This could be replaced with a dictionary that links
> command names to functions.
>
> 837: "while True:"
>
> 842-843: Unnecessary except clause.
>
> 844: Sidenote: The check for len(value) seems unneeded
>
> 858: "for be_item in be_list:" (and replace "be_list[idx]" ->
> "be_item" in the remainder of the for loop.
>
> 888: "for beVals in be_list:" (also, change "beVals" to "be_val" or
> something similar
>
> 894: "is not None:"
>
>
>
> Luis de Bethencourt wrote:
>> Hi,
>>
>> Could I please ask for reviewing the 2.4->2.6 & PEP8 changes
>> for beadm specific files ?
>>
>> Changes are covered by following bug:
>> 11680 - The libbe Python bridge needs to be compiled against
>> libpython2.6
>> 11679 - beadm should be updated to work with Python version 2.6
>>
>> Thank you very much,
>> Jan
>>
>>
>> * Webrev
>> ========
>> http://cr.opensolaris.org/~evanl/11679/
>>
>>
>> * Testing
>> =========
>> * platforms tested: x86
>> * run regression tests
>>
>>
>> * pylint results:
>> ================= before:
>> beadm.py -3.77
>> BootEnvironment.py -4.46
>> messages.py -9.29
>>
>> after:
>> beadm.py 7.84
>> BootEnvironment.py 8.02
>> messages.py 5.6
>>
>>
>> messages.py needs an extreme make over, a new bug will be open for
>> this shortly.
>>
>>
>> Luis
>>
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
