-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix bug with w- io mode #1795
fix bug with w- io mode #1795
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1795 +/- ##
=======================================
Coverage 91.97% 91.97%
=======================================
Files 27 27
Lines 2617 2618 +1
Branches 683 683
=======================================
+ Hits 2407 2408 +1
Misses 138 138
Partials 72 72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Good catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Could use a test if it's not too difficult to put one together
We could add the following test: import unittest
import tempfile
from pathlib import Path
from pynwb import NWBHDF5IO
from pynwb.testing.mock.file import mock_NWBFile
class TestNWBHDF5IO(unittest.TestCase):
def test_file_creation_and_deletion(self):
io_modes_that_create_file = ["w", "w-", "x"]
with tempfile.TemporaryDirectory() as temp_dir:
temp_dir = Path(temp_dir)
for io_mode in io_modes_that_create_file:
file_path = temp_dir / f"test_io_mode={io_mode}.nwb"
# Test file creation
nwbfile = mock_NWBFile()
with NWBHDF5IO(str(file_path), io_mode) as io:
io.write(nwbfile)
if __name__ == "__main__":
unittest.main() Could this be a good test? if so, where should it be? |
@h-mayorquin I think that test looks fine to cover this use case. Please add the test to this test class https://github.com/NeurodataWithoutBorders/pynwb/blob/dev/tests/integration/hdf5/test_io.py#L19 |
@rly added the test. |
Thanks for adding the test. I marked this PR as ready for review. @h-mayorquin I hope that's OK. @bendichter please re-review. |
Motivation
@bendichter
The PR in #1748 introduced a bug where the io mode "w-" still tries to load the name space. See coments:
https://github.com/NeurodataWithoutBorders/pynwb/pull/1748/files#r1421748605
https://github.com/NeurodataWithoutBorders/pynwb/pull/1748/files#r1421748559
How to test the behavior?
Currently this fails in dev / main
The current PR fixes this.
Checklist
flake8
from the source directory.