Ensure that collection file is closed. Resolves #186.

[bdice]

Motivation and Context

Resolves unclosed file error in #186.

Types of Changes

• Bug fix

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the changelog.

[codecov]

Codecov Report

Merging #187 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   65.03%   65.03%   +<.01%     
==========================================
  Files          37       37              
  Lines        5531     5532       +1     
==========================================
+ Hits         3597     3598       +1     
  Misses       1934     1934

Impacted Files	Coverage Δ
signac/contrib/collection.py	88.9% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a16292...2a8f3cd. Read the comment docs.

[codecov]

Codecov Report

Merging #187 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   65.03%   65.03%   +<.01%     
==========================================
  Files          37       37              
  Lines        5531     5532       +1     
==========================================
+ Hits         3597     3598       +1     
  Misses       1934     1934

Impacted Files	Coverage Δ
signac/contrib/collection.py	88.9% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a16292...2a8f3cd. Read the comment docs.

[csadorf]

Can you add a unit test that catches the bug, please?

[bdice]

@csadorf I'm not sure how I would do that. Would you want to somehow ensure that a ResourceWarning is never raised during the execution of the tests?

[bdice]

Update: I found a way. This is an example of the warning filter I will add. https://stackoverflow.com/questions/24717027/convert-python-3-resourcewarnings-into-exception/25067818#25067818

[bdice]

I wasn't able to make this work. I imported warnings and edited the test as follows:

    def test_read(self):
        with Collection.open(self._fn_collection, mode='r') as c:
            self.assertEqual(len(list(c)), 3)
        with open(self._fn_collection, 'a') as file:
            file.write("{'a': 0}\n")      # ill-formed JSON (single quotes instead of double quotes)
        with self.assertRaises(JSONParseError):
            with warnings.catch_warnings():
                warnings.simplefilter('error', ResourceWarning)
                with Collection.open(self._fn_collection, mode='r') as c:
                    pass

I got the following output:

$ python test_collection.py -v FileCollectionTestBadJson.test_read
test_read (__main__.FileCollectionTestBadJson) ... test_collection.py:617: ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/signac_collection_7dr7s4cy/test.txt' mode='r' encoding='UTF-8'>
  pass
ok

----------------------------------------------------------------------
Ran 1 test in 0.010s

OK

I can't get this warning to raise an error even though I directed the warnings module to do so in the filter. I googled for a little bit but I'm not coming up with anything helpful for resolving this.

[csadorf]

I can't get this warning to raise an error even though I directed the warnings module to do so in the filter. I googled for a little bit but I'm not coming up with anything helpful for resolving this.

I don't see why that wouldn't work either... :/ I'm ok with cutting our losses and merge as is.