[PATCH] 58 Use 'with' construct for file handling (Also added finally block when an exception is to be handled) #Ticket 562
Please review he attached patch with a minor modification in code - to use the with construct for handling files. --Abhishek
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'. -- Endi S. Dewata
On Fri, 2013-05-24 at 12:56 -0500, Endi Sukma Dewata wrote:
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.
Both NSSConnection and httplib.HTTPConnection do not support Context Manager. Hence, no changes made here.
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
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. --Abhishek
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. -- Endi S. Dewata
Fixed and pushed to master. --Abhsihek On Wed, 2013-06-05 at 19:44 -0500, Endi Sukma Dewata wrote:
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.
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata