The updated webrev looks good. Thanks, Keith
Karen Tung wrote: > > On 01/15/10 16:16, Keith Mitchell wrote: >> Now that I think about it, you should define char_set as a constant >> of the install_utils.py. Everything else looks great. >> > Hi Keith, > > I updated the install_utils.py file with your comment above. > Please see the updated webrev at: > > http://cr.opensolaris.org/~ktung/12726-updated2/ > > Thank you again for your comments. > > --Karen > >> 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 >
