Add method AbstractContainer.get_fields_conf, fix fields handling (#441)

src/hdmf/build/manager.py

@@ -579,7 +579,7 @@ def __get_cls_dict(self, base, addl_fields, name=None, default_name=None):
             else:
                 # if not, add arguments to fields for getter/setter generation
                 dtype = self.__get_type(field_spec)
-                if self.__ischild(dtype):
+                if self.__ischild(dtype) and issubclass(base, Container):
                     fields.append({'name': f, 'child': True})
                 else:
                     fields.append(f)

src/hdmf/common/table.py

@@ -989,7 +989,6 @@ class DynamicTableRegion(VectorData):
 
     __fields__ = (
         'table',
-        'description'
     )
 
     @docval({'name': 'name', 'type': str, 'doc': 'the name of this VectorData'},

src/hdmf/container.py

@@ -1,6 +1,7 @@
 import numpy as np
 from abc import ABCMeta, abstractmethod
 from uuid import uuid4
+from collections import OrderedDict
 from .utils import (docval, get_docval, call_docval_func, getargs, ExtenderMeta, get_data_shape, fmt_docval_args,
                     popargs, LabelledDict)
 from .data_utils import DataIO, append_data, extend_data
@@ -21,6 +22,10 @@ class AbstractContainer(metaclass=ExtenderMeta):
     # Autogenerated properties will store values in self.__field_values
     __fields__ = tuple()
 
+    # This field is automatically set by __gather_fields before initialization.
+    # It holds all the values in __fields__ for this class and its parent classes.
+    __fieldsconf = tuple()
+
     _pconf_allowed_keys = {'name', 'doc', 'settable'}
 
     # Override the _setter factor function, so directives that apply to
@@ -73,6 +78,14 @@ def _check_field_spec(field):
             tmp = {'name': tmp}
         return tmp
 
+    @classmethod
+    def _check_field_spec_keys(cls, field_conf):
+        for k in field_conf:
+            if k not in cls._pconf_allowed_keys:
+                msg = ("Unrecognized key '%s' in %s config '%s' on %s"
+                       % (k, cls._fieldsname, field_conf['name'], cls.__name__))
+                raise ValueError(msg)
+
     @classmethod
     def _get_fields(cls):
         return getattr(cls, cls._fieldsname)
@@ -81,6 +94,10 @@ def _get_fields(cls):
     def _set_fields(cls, value):
         return setattr(cls, cls._fieldsname, value)
 
+    @classmethod
+    def get_fields_conf(cls):
+        return cls.__fieldsconf
+
     @ExtenderMeta.pre_init
     def __gather_fields(cls, name, bases, classdict):
         '''
@@ -92,21 +109,37 @@ def __gather_fields(cls, name, bases, classdict):
             msg = "'%s' must be of type tuple" % cls._fieldsname
             raise TypeError(msg)
 
-        if len(bases) and 'Container' in globals() and issubclass(bases[-1], Container) \
-                and bases[-1]._get_fields() is not fields:
-            new_fields = list(fields)
-            new_fields[0:0] = bases[-1]._get_fields()
-            cls._set_fields(tuple(new_fields))
-        new_fields = list()
-        docs = {dv['name']: dv['doc'] for dv in get_docval(cls.__init__)}
-        for f in cls._get_fields():
+        # check field specs and create map from field name to field conf dictionary
+        fields_dict = OrderedDict()
+        for f in fields:
             pconf = cls._check_field_spec(f)
-            pname = pconf['name']
-            pconf.setdefault('doc', docs.get(pname))
+            cls._check_field_spec_keys(pconf)
+            fields_dict[pconf['name']] = pconf
+        all_fields_conf = list(fields_dict.values())
+
+        # check whether this class overrides __fields__
+        if len(bases):
+            base_fields = bases[-1]._get_fields()  # tuple of field names from base class
+            if base_fields is not fields:
+                # check whether new fields spec already exists in base class
+                for field_name in fields_dict:
+                    if field_name in base_fields:
+                        raise ValueError("Field '%s' cannot be defined in %s. It already exists on base class %s."
+                                         % (field_name, cls.__name__, bases[-1].__name__))
+                # prepend field specs from base class to fields list of this class
+                all_fields_conf[0:0] = bases[-1].get_fields_conf()
+
+        # create getter and setter if attribute does not already exist
+        # if 'doc' not specified in __fields__, use doc from docval of __init__
+        docs = {dv['name']: dv['doc'] for dv in get_docval(cls.__init__)}
+        for field_conf in all_fields_conf:
+            pname = field_conf['name']
+            field_conf.setdefault('doc', docs.get(pname))
             if not hasattr(cls, pname):
-                setattr(cls, pname, property(cls._getter(pconf), cls._setter(pconf)))
-            new_fields.append(pname)
-        cls._set_fields(tuple(new_fields))
+                setattr(cls, pname, property(cls._getter(field_conf), cls._setter(field_conf)))
+
+        cls._set_fields(tuple(field_conf['name'] for field_conf in all_fields_conf))
+        cls.__fieldsconf = tuple(all_fields_conf)
 
     def __new__(cls, *args, **kwargs):
         inst = super().__new__(cls)
@@ -274,46 +307,45 @@ def _setter(cls, field):
         """Returns a list of setter functions for the given field to be added to the class during class declaration."""
         super_setter = AbstractContainer._setter(field)
         ret = [super_setter]
-        if isinstance(field, dict):
-            # check keys
-            for k in field.keys():
-                if k not in cls._pconf_allowed_keys:
-                    msg = "Unrecognized key '%s' in __field__ config '%s' on %s" % (k, field['name'], cls.__name__)
-                    raise ValueError(msg)
-
-            # create setter with check for required name
-            if field.get('required_name', None) is not None:
-                name = field['required_name']
-                idx1 = len(ret) - 1
-
-                def container_setter(self, val):
-                    if val is not None and val.name != name:
-                        msg = "%s field on %s must be named '%s'" % (field['name'], self.__class__.__name__, name)
+        # create setter with check for required name
+        if field.get('required_name', None) is not None:
+            name = field['required_name']
+            idx1 = len(ret) - 1
+
+            def container_setter(self, val):
+                if val is not None:
+                    if not isinstance(val, AbstractContainer):
+                        msg = ("Field '%s' on %s has a required name and must be a subclass of AbstractContainer."
+                               % (field['name'], self.__class__.__name__))
+                        raise ValueError(msg)
+                    if val.name != name:
+                        msg = ("Field '%s' on %s must be named '%s'."
+                               % (field['name'], self.__class__.__name__, name))
                         raise ValueError(msg)
-                    ret[idx1](self, val)
-
-                ret.append(container_setter)
-
-            # create setter that accepts a value or tuple, list, or dict or values and sets the value's parent to self
-            if field.get('child', False):
-                idx2 = len(ret) - 1
-
-                def container_setter(self, val):
-                    ret[idx2](self, val)
-                    if val is not None:
-                        if isinstance(val, (tuple, list)):
-                            pass
-                        elif isinstance(val, dict):
-                            val = val.values()
-                        else:
-                            val = [val]
-                        for v in val:
-                            if not isinstance(v.parent, Container):
-                                v.parent = self
-                            # else, the ObjectMapper will create a link from self (parent) to v (child with existing
-                            # parent)
-
-                ret.append(container_setter)
+                ret[idx1](self, val)
+
+            ret.append(container_setter)
+
+        # create setter that accepts a value or tuple, list, or dict or values and sets the value's parent to self
+        if field.get('child', False):
+            idx2 = len(ret) - 1
+
+            def container_setter(self, val):
+                ret[idx2](self, val)
+                if val is not None:
+                    if isinstance(val, (tuple, list)):
+                        pass
+                    elif isinstance(val, dict):
+                        val = val.values()
+                    else:
+                        val = [val]
+                    for v in val:
+                        if not isinstance(v.parent, Container):
+                            v.parent = self
+                        # else, the ObjectMapper will create a link from self (parent) to v (child with existing
+                        # parent)
+
+            ret.append(container_setter)
         return ret[-1]
 
     def __repr__(self):