http://cr.opensolaris.org/~luisbg/11680_2/
Fix: isinstance("*", str)
Luis
Luis de Bethencourt wrote:
> 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
>
>
