BUG: allow repeated mol2 writes #2680
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2680 +/- ##
========================================
Coverage 91.11% 91.11%
========================================
Files 175 175
Lines 23637 23642 +5
Branches 3090 3090
========================================
+ Hits 21536 21541 +5
Misses 1482 1482
Partials 619 619
Continue to review full report at Codecov.
|
|
|
||
| def test_mol2_multi_write(): | ||
| # see: gh-2678 | ||
| with tempdir.TempDir() as tmpdir: |
There was a problem hiding this comment.
Maybe use the tmpdir fixture from pytest here?
There was a problem hiding this comment.
It doesn't seem to leave any files behind and there are other uses in our code base--what's the justification for using one over the other?
I had assumed our own solution was there for a reason.
There was a problem hiding this comment.
i.e., if testsuite/MDAnalysisTests/tempdir.py shouldn't be used, may it should be removed or deprecated or whatever
There was a problem hiding this comment.
I see it is just copied from other examples in the same module, so if a broader cleanup is in order that's probably a separate task, though I'm happy to change this specific case...
There was a problem hiding this comment.
I had assumed our own solution was there for a reason.
Yeah, I'm not sure why it's preferred over the fixture. It looks like a pretty straightforward context manager, although there's a comment that created files might not be deleted on Windows if they're still open. I guess one advantage it has is that you can add a prefix or suffix to the directory name, although I don't think that is ever used in the test suite. There's also a decorator that is used twice. Either way I think pytest's fixture can probably replace all instances of the tempdir module.
I also like with tmpdir.as_cwd() because it's clearer what is happening. You don't need to outfile = os.path.join(str(tmpdir), 'group1.mol2') because you've changed directories, but that's not immediately obvious.
* add a unit test reproduce MDAnalysisgh-2678 * the `encode_block` method of `MOL2Writer` now restores mutated properties of `molecule` to prevent the issue
tylerjereddy
force-pushed
the
issue_2678_mol2_repeat
branch
from
dd81b27 to
97408d7
Compare
|
OK, revised to use I'll open a separate issue for removing the in-house |
|
Thank you @tylerjereddy! |
Fixes #2678
Changes made in this Pull Request:
add a unit test reproduce Can only write mol2 file once #2678
the
encode_blockmethod ofMOL2Writernowrestores mutated properties of
moleculeto preventthe issue
PR Checklist