Now that I think about it, you should define char_set as a constant of the install_utils.py. Everything else looks great.
Thanks, Keith Karen Tung wrote: > On 01/15/10 08:45 AM, Keith Mitchell wrote: >> Hi Karen, >> >> Karen Tung wrote: >>> Hi Keith, >>> >>> Thank you so much for the code review. Please see my responses below: >>> >>>> >>>> install_utils.py: >>>> >>>> 695: The 'finally' clause will close this file before the return, >>>> so this line isn't needed. (Did you consider using the "with" >>>> syntax for opening files? I find it provides a cleaner way to >>>> manage opening/closing files.) >>> >>> Since I am returning at line 696, I am not sure whether the finally >>> clause will get executed. I know the >>> finally clause will get executed if we continue with the rest of the >>> function. I am not sure what will >>> happen if I return. I do it here immediately >>> before I return to make sure. So, you are sure that it will? If >>> so, I can remove this. >> >> "finally" clauses always get executed, no matter how the 'try' clause >> exits (whether it exits 'normally,' from an exception, or via return >> statement). >> >> - Keith > Hi Keith, > > Thanks for the explaination. > I changed the file processing code to use the "with" syntax. > > An updated webrev with the changes I made addressing your comments and > Dave's comments > is here: > > http://cr.opensolaris.org/~ktung/12726-updated/ > > Let me know if you have further comments. > > Thanks, > > --Karen
