add logic to erase file if writing fails #912
Conversation
| if not nwbfile_loaded_succesfully and not file_initially_exists: # Not sure we need this | ||
| nwbfile_path_in.unlink() | ||
|
|
||
| if not nwbfile_written_succesfully and not file_initially_exists: |
There was a problem hiding this comment.
Aside from better variable naming, the key difference in this PR is this line by limiting the global notion of 'success' to just 'success during write process' and if file didn't initially exist but write started and failed, try to clean it up
Emphasis on 'try' since not guaranteed to succeed without error
| except Exception as write_error: | ||
| nwbfile_written_succesfully = False | ||
| raise write_error |
There was a problem hiding this comment.
I see, you also added a capture block here to further scope the success of the operation
There was a problem hiding this comment.
Right. I think this is the core. Separating errors from loading or creating from errors of writing.
But as you were mentioning maybe us handling unlinking is not a good idea ...
There was a problem hiding this comment.
In this case we're attempting to leave the system as we left it, which is fine
The case I am against is what you don't have here
if any_error_condition and file_initially_exists:
nwbfile_path_in.unlink()
CodyCBakerPhD
merged commit a2dadd1
into
fix_overwrite_on_corrupt_file
Discussion from issue #910. @CodyCBakerPhD, this is what I had in mind.