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:
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
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.
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
