Hi, Fixes and review suggestions have been done, and can be viewed/reviewed at:
http://cr.opensolaris.org/~evanl/11679v2/ Thanks for the help! 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
