Fix PDBWriter with StringIO for invalid coordinates #2518
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2518 +/- ##
========================================
Coverage 90.41% 90.41%
========================================
Files 170 170
Lines 23150 23150
Branches 2990 2990
========================================
Hits 20930 20930
Misses 1621 1621
Partials 599 599
Continue to review full report at Codecov.
|
| @@ -678,6 +682,10 @@ def _check_pdb_coordinates(self): | |||
| except OSError as err: | |||
| if err.errno == errno.ENOENT: | |||
| pass | |||
| except TypeError: | |||
| if isinstance(self.filename, StringIO): | |||
| pass | |||
There was a problem hiding this comment.
This is equivalent to
except TypeError:
passShouldn't it be the following?
except TypeError:
if isinstance(self.filename, StringIO):
pass
else:
raiseThere was a problem hiding this comment.
Definitely! Thanks for spotting this problem!
There was a problem hiding this comment.
The same logic should apply to the previous exception as well, which is suffering from the same problem.
However, this rises a question: should we ignore all the exceptions that can occur while trying os.remove so that the correct exception ValueError (which is the problem triggering os.remove) is thrown instead?
There was a problem hiding this comment.
We can at least wonder what would be the equivalent of an os.remove on a StringIO. Now, we have an odd discrepancy in behaviour between files and StringIO. Maybe it should be a different issue?
There was a problem hiding this comment.
I think the equivalent of os.remove for StringIO is just getting out of scope. However, it looks like that calling close() already frees the memory buffer:
StringIO.close()
Free the memory buffer. Attempting to do further operations with a closed StringIO object will raise a ValueError.
There was a problem hiding this comment.
I guess, then, that the handling is appropriate.
Fixes #2512
Changes made in this Pull Request:
TypeErrorwhenPDBWriteris trying to remove aStringIOobject with invalid coordinates, so that the correct exception can be raised instead.PR Checklist