Hi, It's looking very good. Just a few follow-up comments.
BootEnvironment.py: 132: Nit: use "<x> not in <y>" instead of "not <x> in <y>". Same for any other changes from 'not <y>.has_key(<x>)' 180: isinstance (I must've missed this first go around, sorry!) beadm.py: 270: Switch to: "@" in be.trgt_be_name_or_snapshot[0] 321, 324: Sorry, I meant to say that these should be aligned with the open parentheses from the previous line. 850, 852, 854: change "be_list[idx]" to "be_item" or this will break 849, 851, 853, 884: "is not None:" All else looks good. Thanks! - Keith Luis de Bethencourt wrote: > 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 > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
