Hi Darren,
On Sep 30, 2011, at 2:05 AM, Darren Kenny wrote:
> Hi Seth,
>
> In general it looks good - I have some comments, but mainly nits TBH and
> could be ignored to avoid churn:
>
> legacygrub.py:
>
> 404 + if (argdict.get('bootfs', None) is None and
> 405 + argdict.get('rpool', None) is None):
>
> - I know it's consistent with the rest of the code that I can see, but I
> wonder why you don't do the following to check if a specific key is in
> the dict():
>
> if 'bootfs' not in argdict and 'rpool' not in argdict:
Originally, it was possible for those keys to exist but have None values;
that's not the case anymore, though, so I'll change it.
>
>
> menulst.py:
>
> 132 + if idx is -1:
> 133 + idx = self._find_command_index('title')
> 134 + if idx is not -1:
> 135 + self._cmdlist.insert(idx + 1, mlcmd)
> 136 + return
>
> - I'm not sure that I like the use of 'is' and 'is not' with integers,
> while it does appear to work, it's certainly not in keeping with normal
> Python coding practice, and even conflicts with similar code on the
> lines directly preceding the above:
>
> 127 + if idx != -1:
> 128 + self._cmdlist.insert(idx + 1, mlcmd)
> 129 + return
>
That's true -- I've made them consistent.
>
> bootconfig.py:
>
> 516 + if len(default_instances) == 0:
>
> - While this works, it's common to use simply 'if not default_instances:'
> to test this. This checks for both the value being None, and the list
> (and strings BTW) having a length of 0.
Yea, I know but all I really want to test is if default_instances has 0 items
-- I know it's not None. I'm constantly torn between Pythonisms and
readability of code ;).
Thanks,
--S
>
> Thanks,
>
> Darren.
>
> On 30/09/2011 05:55, Seth Goldberg wrote:
>> Hi,
>>
>> Can I please have a few reviewers for:
>>
>> https://cr.opensolaris.org/action/browse/caiman/sethg/7096624/webrev/
>>
>> This is a stopper and it's been fully tested (100% code coverage of the
>> changes verified with the coverage tool) with a variety of menu.lst files.
>>
>> Thanks,
>> --S
>> _______________________________________________
>> caiman-discuss mailing list
>> [email protected]
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
