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
