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
