Ensure file is closed promptly in case of error #35587
Conversation
|
copied the milestone from #35567. feel free to amend as appropriate. |
|
Added the What's New note. |
pandas/io/sas/sas_xport.py
Outdated
| self._read_header() | ||
| try: | ||
| self._read_header() | ||
| except: # noqa: E722 OK because exception re-raised |
There was a problem hiding this comment.
never use bare exceptions, use
try:
....
except Exception:
# close on error reading header...
....
There was a problem hiding this comment.
I can do that, but what about errors that don't inherit from Exception?
I note that the PEP says:
A good rule of thumb is to limit use of bare 'except' clauses to two cases:
If the exception handler will be printing out or logging the traceback; at least the user will be aware that an error has occurred.
If the code needs to do some cleanup work, but then lets the exception propagate upwards with raise. try...finally can be a better way to handle this case.
(try...finally obviously isn't applicable here since we only want to close on error)
There was a problem hiding this comment.
OK, I note that exceptions that don't inherit from Exception are supposed to be system-exiting, so we can suppose some higher-level process will clean up for us in this case.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I'm fine with this implementation, but curious: wouldnt the idiomatic way to do this be with a contextmanager? |
| reader.close() | ||
| return data | ||
| try: | ||
| return reader.read() |
There was a problem hiding this comment.
is this needed that you are catching error (and closing) at the lower level? maybe only catch here
There was a problem hiding this comment.
The close at the lower level (sas7bdat.py, sas_xport.py) is to match with the get_filepath_or_buffer call 10 lines earlier, and also with various calls to close scatter around those files.
It is possible (if both 'lower level' file are only ever used via sasreader.py) that all close calls are unnecessary lower down, but I don't think they do any harm so I would prefer to leave the code symmetrical for the moment (open/close) and potentially get rid of all the close calls in a separate PR.
What do you think?
Fixes #35566. Replaces #35567.