Keith Mitchell wrote: > > > Evan Layton wrote: >> I need to get reviews for the following bugs that are causing failures >> if the directories where the boot/grub menu files live are missing >> (/rpool/boot and /rpool/boot/grub). Also this fix resolves the issue >> with the add_splash_image_to_grub_menu that was causing a failure to >> create a menu.lst file when it's missing. > > Hi Evan, > > Depending on how much time/effort you have to put into items indirectly > related to your changes, here are some suggestions for "Python-izing" > ict.py a little. I tried to limit myself to code that you touched. > > I know ict.py is one of those "C-like" Python files we have. Since > you're touching some of it, would it be possible to change, for example, > lines 329-330 and 321-322 to raise a ValueError instead of forcing a > sys.exit? For this change to mesh well and get called from C code, > exec_ict would need to be changed to be a try/except, log the traceback > appropriately, and set status=1. (Oh and side-note: the if __name__ == > '__main__' block could be completely changed to: > > if __name__ == '__main__' and sys.argv[2:] and sys.argv[1] in dir(ict): > exec_ict(*sys.argv) > > The * operator expands an array into a tuple for passing to a function. > (Actually, the "sys.argv[2:] and sys.argv[1] in dir(ict)" might be worth > leaving out, so that someone who calls into ict.py with incorrect > parameters at least gets a traceback and instead of being silently ignored) > > Also, and this is completely unrelated to the bugs, but line 339 should > probably be: > self.IS_SPARC = (platform.processor() == "sparc") - platform.processor() > is essentially "uname -p" > > Line 345: Should be "except ValueError:" (otherwise it would catch > KeyboardInterrupts, SystemExits, etc.) > > Once all other code review comments have been addressed, change all tabs > to spaces (per PEP 8). And include a pylint score (even if it's bad - > perhaps a "before" and "after"?) > > I should stop before I get distracted and try to rewrite the entire > thing... > > - Keith > >> >> >> The bugs: >> http://defect.opensolaris.org/bz/show_bug.cgi?id=7880 >> http://defect.opensolaris.org/bz/show_bug.cgi?id=11436 >> >> >> The webrev: >> http://cr.opensolaris.org/~evanl/7880/ >> >> >> Thanks, >> -evan >> >> _______________________________________________ >> 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
Hey Keith and Evan Regarding Pythonizing ict.py. I think it would be valid to do that when addressing Bug 6256 - "ICT - Convert remaining ICT implemented in "C" to Python" I suggest adding a comment to bug 6256 stating that ict.py should be more Python-ish/OOD-ish. I don't think that should be done as part of this fix. Joe
