On 5/21/2013 12:02 PM, Abhishek Koneru wrote: > Please review he attached patch with a minor modification in code - to > use the with construct for handling files. Some of the 'try-except-finally' clauses probably can be simplified because sometimes the 'except' only wraps the original exception, or logs an error, or does simple error handling. It's also possible to nest 'with' inside 'try-except' (or the other way around); one to handle the resource and the other to handle the error. 1. In drmclient.py, in http_request() and https_request() suppose the NSSConnection supports Context Manager we could use 'with' statement too. Could you check? http://docs.python.org/2/reference/compound_stmts.html#with 2. Also in http_request() and https_request(), I'm not sure if we really need to wrap the original exception, but that's a separate issue. If we can remove it we can use 'with' here, or at least nest it.

3. In kra.__init__() the self.password = '' assignment can be moved before open() so in case of error it will be blank already. This way we can use the 'with' statement.

4. In pkimanifest.py, in file.write() and read() the error logging should really be done by the caller. But here at least we can use 'with' nested inside the 'try-except'.

On 5/31/2013 2:20 PM, Abhishek Koneru wrote: >> 1. In drmclient.py, in http_request() and https_request() suppose the >> NSSConnection supports Context Manager we could use 'with' statement >> too. Could you check? >> >> http://docs.python.org/2/reference/compound_stmts.html#with >> >> 2. Also in http_request() and https_request(), I'm not sure if we really >> need to wrap the original exception, but that's a separate issue. If we >> can remove it we can use 'with' here, or at least nest it. > > Both NSSConnection and httplib.HTTPConnection do not support Context > Manager. Hence, no changes made here. OK. >> 3. In kra.__init__() the self.password = '' assignment can be moved >> before open() so in case of error it will be blank already. This way we >> can use the 'with' statement. > -- Nested the with in the try-except block There's a trailing space on line 419. Please fix before push. >> 4. In pkimanifest.py, in file.write() and read() the error logging >> should really be done by the caller. But here at least we can use 'with' >> nested inside the 'try-except'. > used with for file operations >> > Please review the attached patch. ACK.