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. Thanks! -evan
