diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e15d5b78..bed5c6300 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ - Allow manual triggering of some GitHub Actions. @rly ([#744](https://github.com/hdmf-dev/hdmf/pull/744)) - Relax input validation of `HDF5IO` to allow for s3fs support. Existing arguments of `HDF5IO` are modified as follows: i) `mode` was given a default value of "r", ii) `path` was given a default value of `None`, and iii) `file` can now accept an `S3File` type argument. @bendichter ([#746](https://github.com/hdmf-dev/hdmf/pull/746)) - Added ability to create and get back handle to empty HDF5 dataset. @ajtritt (#747) +- Added `AbstractContainer._in_construct_mode` that is set and modified only by the ObjectMapper when constructing an + object from a builder read from a file. Subclasses of `AbstractContainer` can check `_in_construct_mode` + during the initialization phase as part of ``__init__`` to distinguish between actions during construction + (i.e., read from disk) vs. creation by the user, e.g., to determine whether to raise a warning or error when + encountering invalid data to support reading and correcting data that is invalid while preventing creation + of new data that is invalid. @rly (#751) ### Bug fixes - Fixed PyNWB dev CI. @rly ([#749](https://github.com/hdmf-dev/hdmf/pull/749)) diff --git a/docs/gallery/plot_external_resources.py b/docs/gallery/plot_external_resources.py index b9ad73df6..55c694634 100644 --- a/docs/gallery/plot_external_resources.py +++ b/docs/gallery/plot_external_resources.py @@ -505,13 +505,13 @@ table = pd.read_sql_query("SELECT * from %s" % table_name, db) table.set_index('id', inplace=True) ref_table = getattr(er, table_name).to_dataframe() - assert(np.all(np.array(table.index) == np.array(ref_table.index) + 1)) + assert np.all(np.array(table.index) == np.array(ref_table.index) + 1) for c in table.columns: # NOTE: SQLite uses 1-based row-indices so we need adjust for that if np.issubdtype(table[c].dtype, np.integer): - assert(np.all(np.array(table[c]) == np.array(ref_table[c]) + 1)) + assert np.all(np.array(table[c]) == np.array(ref_table[c]) + 1) else: - assert(np.all(np.array(table[c]) == np.array(ref_table[c]))) + assert np.all(np.array(table[c]) == np.array(ref_table[c])) cursor.close() ############################################################################### diff --git a/setup.cfg b/setup.cfg index 75418aa41..253d4ffc0 100644 --- a/setup.cfg +++ b/setup.cfg @@ -19,9 +19,10 @@ exclude = versioneer.py src/hdmf/_version.py src/hdmf/_due.py + docs/source/tutorials/ + docs/_build/ per-file-ignores = docs/gallery/*:E402,T001 - docs/source/tutorials/*:E402,T001 src/hdmf/__init__.py:F401 src/hdmf/backends/__init__.py:F401 src/hdmf/backends/hdf5/__init__.py:F401 diff --git a/src/hdmf/build/objectmapper.py b/src/hdmf/build/objectmapper.py index 115ff3070..a9e3cf8e6 100644 --- a/src/hdmf/build/objectmapper.py +++ b/src/hdmf/build/objectmapper.py @@ -1254,8 +1254,12 @@ def construct(self, **kwargs): def __new_container__(self, cls, container_source, parent, object_id, **kwargs): """A wrapper function for ensuring a container gets everything set appropriately""" - obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id) + obj = cls.__new__(cls, container_source=container_source, parent=parent, object_id=object_id, + in_construct_mode=True) + # obj has been created and is in construction mode, indicating that the object is being constructed by + # the automatic construct process during read, rather than by the user obj.__init__(**kwargs) + obj._in_construct_mode = False # reset to False to indicate that the construction of the object is complete return obj @docval({'name': 'container', 'type': AbstractContainer, diff --git a/src/hdmf/container.py b/src/hdmf/container.py index 45eee64b8..b00b91145 100644 --- a/src/hdmf/container.py +++ b/src/hdmf/container.py @@ -176,6 +176,13 @@ def __gather_fields(cls, name, bases, classdict): cls.__fieldsconf = tuple(all_fields_conf) def __new__(cls, *args, **kwargs): + """ + Static method of the object class called by Python to create the object first and then + __init__() is called to initialize the object's attributes. + + NOTE: this method is called directly from ObjectMapper.__new_container__ during the process of + constructing the object from builders that are read from a file. + """ inst = super().__new__(cls) if cls._experimental: warn(_exp_warn_msg(cls)) @@ -184,6 +191,9 @@ def __new__(cls, *args, **kwargs): inst.__children = list() inst.__modified = True inst.__object_id = kwargs.pop('object_id', str(uuid4())) + # this variable is being passed in from ObjectMapper.__new_container__ and is + # reset to False in that method after the object has been initialized by __init__ + inst._in_construct_mode = kwargs.pop('in_construct_mode', False) inst.parent = kwargs.pop('parent', None) return inst diff --git a/tests/unit/test_container.py b/tests/unit/test_container.py index f32897e70..a6d452910 100644 --- a/tests/unit/test_container.py +++ b/tests/unit/test_container.py @@ -1,4 +1,6 @@ import numpy as np +from uuid import uuid4, UUID + from hdmf.container import AbstractContainer, Container, Data from hdmf.testing import TestCase from hdmf.utils import docval @@ -10,22 +12,47 @@ class Subcontainer(Container): class TestContainer(TestCase): - def test_constructor(self): - """Test that constructor properly sets parent and both child and parent have an object_id + def test_new(self): + """Test that __new__ properly sets parent and other fields. """ parent_obj = Container('obj1') - child_obj = Container.__new__(Container, parent=parent_obj) + child_object_id = str(uuid4()) + child_obj = Container.__new__(Container, parent=parent_obj, object_id=child_object_id, + container_source="test_source") self.assertIs(child_obj.parent, parent_obj) self.assertIs(parent_obj.children[0], child_obj) - self.assertIsNotNone(parent_obj.object_id) - self.assertIsNotNone(child_obj.object_id) + self.assertEqual(child_obj.object_id, child_object_id) + self.assertFalse(child_obj._in_construct_mode) + self.assertTrue(child_obj.modified) - def test_constructor_object_id_none(self): - """Test that setting object_id to None in __new__ is OK and the object ID is set on get + def test_new_object_id_none(self): + """Test that passing object_id=None to __new__ is OK and results in a non-None object ID being assigned. """ parent_obj = Container('obj1') child_obj = Container.__new__(Container, parent=parent_obj, object_id=None) self.assertIsNotNone(child_obj.object_id) + UUID(child_obj.object_id, version=4) # raises ValueError if invalid + + def test_new_construct_mode(self): + """Test that passing in_construct_mode to __new__ sets _in_construct_mode and _in_construct_mode can be reset. + """ + parent_obj = Container('obj1') + child_obj = Container.__new__(Container, parent=parent_obj, object_id=None, in_construct_mode=True) + self.assertTrue(child_obj._in_construct_mode) + child_obj._in_construct_mode = False + self.assertFalse(child_obj._in_construct_mode) + + def test_init(self): + """Test that __init__ properly sets object ID and other fields. + """ + obj = Container('obj1') + self.assertIsNotNone(obj.object_id) + UUID(obj.object_id, version=4) # raises ValueError if invalid + self.assertFalse(obj._in_construct_mode) + self.assertTrue(obj.modified) + self.assertEqual(obj.children, tuple()) + self.assertIsNone(obj.parent) + self.assertEqual(obj.name, 'obj1') def test_set_parent(self): """Test that parent setter properly sets parent diff --git a/tests/unit/test_io_hdf5_h5tools.py b/tests/unit/test_io_hdf5_h5tools.py index 100d96902..74342ebbd 100644 --- a/tests/unit/test_io_hdf5_h5tools.py +++ b/tests/unit/test_io_hdf5_h5tools.py @@ -1053,7 +1053,7 @@ def tearDown(self): # Close all the files for i in self.io: i.close() - del(i) + del i self.io = None self.f = None # Make sure the files have been deleted @@ -1282,7 +1282,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1316,7 +1316,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1358,7 +1358,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1390,7 +1390,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path) @@ -1459,7 +1459,7 @@ def setUp(self): def tearDown(self): if self.io is not None: self.io.close() - del(self.io) + del self.io if os.path.exists(self.path): os.remove(self.path)