f kwargs to actual parameters

Summary

Fixes issue #54 (closed)

Focus

Refactoring of `add_parent` (Entity and childs)
Refactoring of `add_property` 

Test Environment

`tox`

Check List for the Author

Please, prepare your MR for a review. Be sure to write a summary and a focus and create gitlab comments for the reviewer. They should guide the reviewer through the changes, explain your changes and also point out open questions. For further good practices have a look at our review guidelines

• All automated tests pass
• Reference related Issues
• Up-to-date CHANGELOG.md
• Annotations in code (Gitlab comments)
  • Intent of new code
  • Problems with old code
  • Why this implementation?

Check List for the Reviewer

• I understand the intent of this MR
• All automated tests pass
• Up-to-date CHANGELOG.md
• The test environment setup works and the intended behavior is reproducible in the test environment
• In-code documentation and comments are up-to-date.
• Check: Are there spezifications? Are they satisfied?

For further good practices have a look at our review guidelines.

src/caosdb/common/models.py

@@ -339,32 +339,54 @@ class Entity(object):
 
         return self
 
-    def add_property(self, property=None, value=None, **kwargs):  # @ReservedAssignment
+    def add_property(self, property=None, value=None, id=None, name=None, description=None, datatype=None,
+                     unit=None, importance=None, inheritance=None):  # @ReservedAssignment
         """Add a property to this entity.
 
         The first parameter is meant to identify the property entity. So the method expects an instance of
         Entity, an integer or a string here. The second parameter is the value of the new property. Any
-        other named parameter may be passed by means of the **kwargs. Accepted keywords are:
+        other named parameter may be passed by means of the keywwords. Accepted keywords are:
         id, name, description, importance, inheritance, datatype, and unit. Any other keyword will be
         ignored right now. But that may change in the future.
 
-        @param property: An identifying parameter (name, id or abstract property).
-        @param value: The value of the new property.
-        @param **kwargs: Any other specification for this property. Accepted keywords: id, name, description, importance, inheritance, datatype, and unit.
-        @raise UserWarning:
-        If the first parameter is None then kwargs['id'] or kwargs['name'] must be defined and not be None.
-        Otherwise a UserWarning is raised.
+        Parameters
+        ----------
+        property : int, str, Property, optional
+            An identifying parameter, by default None
+        value : int, str, Property, optional
+            The value of the new property, by default None
+        id : int, optional
+            Id of the property, by default None
+        name : str, optional
+            Name of the property, by default None
+        description : str, optional
+            Description of the property, by default None
+        datatype : str, optional
+            Datatype of the property, by default None
+        unit : str, optional
+            Unit of the property, by default None
+        importance :str, optional
+            Importance of the property, by default None
+        inheritance : str, optional
+            Inheritance of the property, by default None
 
-        If the first parameter is an integer then it is interpreted as the id and kwargs['id'] must be
-        undefined or None. Otherwise a UserWarning is raised.
+        Returns
+        -------
+        Entity
 
-        If the first parameter is not None and neither an instance of Entity nor an integer it is
-        interpreted as the name and kwargs['name'] must be undefined or None. Otherwise a UserWarning is
-        raised.
+        Raises
+        ------
+        UserWarning
+            If the first parameter is None then id or name must be defined and not be None.
+        UserWarning
+            If the first parameter is an integer then it is interpreted as the id and id must be
+            undefined or None.
+        UserWarning
+             If the first parameter is not None and neither an instance of Entity nor an integer it is
+            interpreted as the name and name must be undefined or None.
         """
-        copy_kwargs = kwargs.copy()
-        name = (kwargs['name'] if 'name' in kwargs else None)
-        pid = (kwargs['id'] if 'id' in kwargs else None)
+
+        pid = id
         abstract_property = None
 
         if isinstance(property, Entity):
@@ -373,20 +395,18 @@ class Entity(object):
             if pid is not None:
                 raise UserWarning("The first parameter was an integer which would normally be interpreted as the id of the property which is to be added. But you have also specified a parameter 'id' in the method call. This is ambiguous and cannot be processed.")
             pid = property
-            copy_kwargs['id'] = pid
+            id = pid
         elif property is not None:
             if name is not None:
                 raise UserWarning("The first parameter was neither an instance of Entity nor an integer. Therefore the string representation of your first parameter would normally be interpreted name of the property which is to be added. But you have also specified a parameter 'name' in the method call. This is ambiguous and cannot be processed.")
             name = str(property)
-            copy_kwargs['name'] = name
 
         if property is None and name is None and pid is None:
             raise UserWarning(
                 "This method expects you to pass at least an entity, a name or an id.")
 
-        del copy_kwargs['importance']
-        del copy_kwargs['inheritance']
-        new_property = Property(**copy_kwargs)
+        new_property = Property(name=name, id=id, description=description, datatype=datatype,
+                                value=value, unit=unit)
 
         if abstract_property is not None:
             new_property._wrap(property)
@@ -399,9 +419,7 @@ class Entity(object):
         new_property.value = value
 
         self.properties.append(
-            property=new_property, importance=(
-                kwargs['importance'] if 'importance' in kwargs else None), inheritance=(
-                    kwargs['inheritance'] if 'inheritance' in kwargs else None))
+            property=new_property, importance=importance, inheritance=inheritance)
 
         return self
 
@@ -426,7 +444,7 @@ class Entity(object):
 
         return self
 
-    def add_parent(self, parent=None, **kwargs):  # @ReservedAssignment
+    def add_parent(self, parent=None, id=None, name=None, inheritance=None):  # @ReservedAssignment
         """Add a parent to this entity.
 
         Parameters
@@ -434,27 +452,23 @@ class Entity(object):
         parent : Entity or int or str or None
             The parent entity, either specified by the Entity object
             itself, or its id or its name. Default is None.
-        **kwargs : dict, optional
-            Additional keyword arguments for specifying the parent by
-            name or id, and for specifying the mode of inheritance.
-
-            id : int
-                Integer id of the parent entity. Ignored if `parent`
-                is not None.
-            name : str
-                Name of the parent entity. Ignored if `parent is not
-                none`.
-            inheritance : str
-                One of ``obligatory``, ``recommended``, ``suggested``, or ``fix``. Specifies the
-                minimum importance which parent properties need to have to be inherited by this
-                entity. If no `inheritance` is given, no properties will be inherited by the child.
-                This parameter is case-insensitive.
-
-                Note that the behaviour is currently not yet specified when assigning parents to
-                Records, it only works for inheritance of RecordTypes (and Properties).
-
-                For more information, it is recommended to look into the
-                :ref:`data insertion tutorial<tutorial-inheritance-properties>`.
+        id : int
+            Integer id of the parent entity. Ignored if `parent`
+            is not None.
+        name : str
+            Name of the parent entity. Ignored if `parent is not
+            none`.
+        inheritance : str
+            One of ``obligatory``, ``recommended``, ``suggested``, or ``fix``. Specifies the
+            minimum importance which parent properties need to have to be inherited by this
+            entity. If no `inheritance` is given, no properties will be inherited by the child.
+            This parameter is case-insensitive.
+
+             Note that the behaviour is currently not yet specified when assigning parents to
+            Records, it only works for inheritance of RecordTypes (and Properties).
+
+            For more information, it is recommended to look into the
+            :ref:`data insertion tutorial<tutorial-inheritance-properties>`.
 
         Raises
         ------
@@ -463,8 +477,8 @@ class Entity(object):
             parameter is passed to this method.
 
         """
-        name = (kwargs['name'] if 'name' in kwargs else None)
-        pid = (kwargs['id'] if 'id' in kwargs else None)
+
+        pid = id
         parent_entity = None
 
         if isinstance(parent, Entity):
@@ -478,9 +492,6 @@ class Entity(object):
             raise UserWarning(
                 "This method expects you to pass at least an entity, a name or an id.")
 
-        inheritance = (kwargs['inheritance']
-                       if 'inheritance' in kwargs else None)
-
         addp = Parent(id=pid, name=name, inheritance=inheritance)
 
         if parent_entity is not None:
@@ -1453,45 +1464,42 @@ class Property(Entity):
 
     """CaosDB's Property object."""
 
-    def add_property(self, property=None, value=None, **kwargs):  # @ReservedAssignment
-        copy_kwargs = kwargs.copy()
-
-        if 'importance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['importance'] = FIX
+    def add_property(self, property=None, value=None, id=None, name=None, description=None, datatype=None,
+                     unit=None, importance=FIX, inheritance=FIX):  # @ReservedAssignment
 
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = FIX
-
-        return super(Property, self).add_property(
-            property=property, value=value, **copy_kwargs)
+        return super().add_property(
+            property=property, id=id, name=name, description=description, datatype=datatype,
+            value=value, unit=unit, importance=importance, inheritance=inheritance)
 
-    def add_parent(self, parent=None, **kwargs):
+    def add_parent(self, parent=None, id=None, name=None, inheritance=FIX):
         """Add a parent Entity to this Property.
 
         Parameters
         ----------
-        parent : Entity or int or str or None, optional
-            The parent entity
-        **kwargs : dict, optional
-            Additional keyword arguments specifying the parent Entity
-            by id or name, and specifying the inheritance level. See
-            :py:meth:`Entity.add_parent` for more information. Note
-            that by default, `inheritance` is set to ``fix``.
+       Parameters
+        ----------
+        parent : Entity or int or str or None
+            The parent entity, either specified by the Entity object
+            itself, or its id or its name. Default is None.
+        id : int
+            Integer id of the parent entity. Ignored if `parent`
+            is not None.
+        name : str
+            Name of the parent entity. Ignored if `parent is not
+            none`.
+        inheritance : str, default: FIX
+            One of ``obligatory``, ``recommended``, ``suggested``, or ``fix``. Specifies the
+            minimum importance which parent properties need to have to be inherited by this
+            entity. If no `inheritance` is given, no properties will be inherited by the child.
+            This parameter is case-insensitive.
 
         See Also
         --------
         Entity.add_parent
 
         """
-        copy_kwargs = kwargs.copy()
 
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = FIX
-
-        return super(Property, self).add_parent(parent=parent, **copy_kwargs)
+        return super(Property, self).add_parent(parent=parent, id=id, name=name, inheritance=inheritance)
 
     def __init__(self, name=None, id=None, description=None, datatype=None,
                  value=None, unit=None):
@@ -1584,21 +1592,14 @@ class RecordType(Entity):
 
     """This class represents CaosDB's RecordType entities."""
 
-    def add_property(self, property=None, value=None, **kwargs):  # @ReservedAssignment
-        copy_kwargs = kwargs.copy()
-
-        if 'importance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['importance'] = RECOMMENDED
+    def add_property(self, property=None, value=None, id=None, name=None, description=None, datatype=None,
+                     unit=None, importance=RECOMMENDED, inheritance=FIX):  # @ReservedAssignment
 
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = FIX
-
-        return super().add_property(property=property, value=value,
-                                    **copy_kwargs)
+        return super().add_property(
+            property=property, id=id, name=name, description=description, datatype=datatype,
+            value=value, unit=unit, importance=importance, inheritance=inheritance)
 
-    def add_parent(self, parent=None, **kwargs):
+    def add_parent(self, parent=None,  id=None, name=None, inheritance=OBLIGATORY):
         """Add a parent to this RecordType
 
         Parameters
@@ -1606,24 +1607,30 @@ class RecordType(Entity):
         parent : Entity or int or str or None, optional
             The parent entity, either specified by the Entity object
             itself, or its id or its name. Default is None.
-        **kwargs : dict, optional
-            Additional keyword arguments specifying the parent Entity by id or
-            name, and specifying the inheritance level. See
-            :py:meth:`Entity.add_parent` for more information. Note
-            that by default, `inheritance` is set to ``obligatory``.
+        Parameters
+        ----------
+        parent : Entity or int or str or None
+            The parent entity, either specified by the Entity object
+            itself, or its id or its name. Default is None.
+        id : int
+            Integer id of the parent entity. Ignored if `parent`
+            is not None.
+        name : str
+            Name of the parent entity. Ignored if `parent is not
+            none`.
+        inheritance : str, default OBLIGATORY
+            One of ``obligatory``, ``recommended``, ``suggested``, or ``fix``. Specifies the
+            minimum importance which parent properties need to have to be inherited by this
+            entity. If no `inheritance` is given, no properties will be inherited by the child.
+            This parameter is case-insensitive.
 
         See Also
         --------
         Entity.add_parent
 
         """
-        copy_kwargs = kwargs.copy()
-
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = OBLIGATORY
 
-        return super().add_parent(parent=parent, **copy_kwargs)
+        return super().add_parent(parent=parent, id=id, name=name, inheritance=inheritance)
 
     def __init__(self, name=None, id=None, description=None, datatype=None):  # @ReservedAssignment
         Entity.__init__(self, name=name, id=id, description=description,
@@ -1640,19 +1647,12 @@ class Record(Entity):
 
     """This class represents CaosDB's Record entities."""
 
-    def add_property(self, property=None, value=None, **kwargs):  # @ReservedAssignment
-        copy_kwargs = kwargs.copy()
-
-        if 'importance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['importance'] = FIX
-
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = FIX
+    def add_property(self, property=None, value=None, id=None, name=None, description=None, datatype=None,
+                     unit=None, importance=FIX, inheritance=FIX):  # @ReservedAssignment
 
         return super().add_property(
-            property=property, value=value, **copy_kwargs)
+            property=property, id=id, name=name, description=description, datatype=datatype,
+            value=value, unit=unit, importance=importance, inheritance=inheritance)
 
     def __init__(self, name=None, id=None, description=None):  # @ReservedAssignment
         Entity.__init__(self, name=name, id=id, description=description,
@@ -1804,19 +1804,12 @@ class File(Record):
 
         return checksum.hexdigest()
 
-    def add_property(self, property=None, value=None, **kwargs):  # @ReservedAssignment
-        copy_kwargs = kwargs.copy()
-
-        if 'importance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['importance'] = FIX
-
-        if 'inheritance' not in copy_kwargs:
-            # set default importance
-            copy_kwargs['inheritance'] = FIX
+    def add_property(self, property=None, id=None, name=None, description=None, datatype=None,
+                     value=None, unit=None, importance=FIX, inheritance=FIX):  # @ReservedAssignment
 
         return super().add_property(
-            property=property, value=value, **copy_kwargs)
+            property=property, id=id, name=name, description=description, datatype=datatype,
+            value=value, unit=unit, importance=importance, inheritance=inheritance)
 
 
 class _Properties(list):