Evan Layton wrote: > Joseph J VLcek wrote: >> 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 >> >> > > Hi Keith, > > I agree with Joe on this that doing the full "pythonification" as part > of this fix is a bit out of scope. However I'll add a comment to the > bug referencing bug 6256 stating that this file needs to be addressed > as you mentioned and I'll add your comments to both 6256 and 11436 as > well.
This works for me. I'll add comments to 6256 that are a bit broader in nature. Thanks! - Keith > > Thanks! > -evan >
