http://cr.opensolaris.org/~luisbg/11680/
Keith Mitchell wrote: > 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 Done. This is a good one. > other changes from 'not <y>.has_key(<x>)' > 180: isinstance (I must've missed this first go around, sorry!) isinstance needs the 2 argument to be a class or an object. python raises an error if it is a string, which is what happens in this case. > beadm.py: > 270: Switch to: "@" in be.trgt_be_name_or_snapshot[0] Done :) > > 321, 324: Sorry, I meant to say that these should be aligned with the > open parentheses from the previous line. No problem. Wasn't really sure what you meant. Better now. > > 850, 852, 854: change "be_list[idx]" to "be_item" or this will break Ooops :S > > 849, 851, 853, 884: "is not None:" very true. done. > > > All else looks good. > > Thanks! > - Keith Thank you for the great review! See you next week :) Luis > > > 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
