refactor: make add_* methods constistently call +=

Author: yt-ms

src/structurizr/model/container.py

@@ -120,7 +120,7 @@ def hydrate(
             parent=software_system,
             technology=container_io.technology,
         )
-        model.add_container(container)
+        model += container
 
         for component_io in container_io.components:
             component = Component.hydrate(component_io, container=container)
@@ -139,7 +139,6 @@ def __iadd__(self, component: Component) -> "Container":
         # TODO: once we move past python 3.6 change to proper return type via
         # __future__.annotations
         if component in self.components:
-            # Nothing to do
             return self
 
         if self.get_component_with_name(component.name):
@@ -155,7 +154,8 @@ def __iadd__(self, component: Component) -> "Container":
                 f"{component.parent}. Cannot add to {self}."
             )
         self._components.add(component)
-        self.get_model().add_component(component)
+        model = self.get_model()
+        model += component
         return self
 
     def get_component_with_name(self, name: str) -> Component:

src/structurizr/model/model.py

@@ -17,15 +17,14 @@
 
 
 import logging
-from typing import Iterable, List, Optional, Union, ValuesView
+from typing import Iterable, List, Optional, Set, ValuesView
 
 from pydantic import Field
 
-from structurizr.model.deployment_node import DeploymentNode, DeploymentNodeIO
+from structurizr.model.deployment_node import DeploymentNode
 
 from ..abstract_base import AbstractBase
 from ..base_model import BaseModel
-from .component import Component
 from .container import Container
 from .container_instance import ContainerInstance
 from .element import Element
@@ -106,20 +105,29 @@ def __init__(
         """
         super().__init__(**kwargs)
         self.enterprise = enterprise
-        self.people = set(people)
-        self.software_systems = set(software_systems)
         self.deployment_nodes = set(deployment_nodes)
         # TODO: simply iterate attributes
         self._elements_by_id = {}
         self._relationships_by_id = {}
         self._id_generator = SequentialIntegerIDGenerator()
 
     def __contains__(self, element: Element):
+        """Return True if the element is in the model."""
         return element in self.get_elements()
 
+    @property
+    def software_systems(self) -> Set[SoftwareSystem]:
+        """Return the software systems in the model."""
+        return set([e for e in self.get_elements() if isinstance(e, SoftwareSystem)])
+
+    @property
+    def people(self) -> Set[Person]:
+        """Return the people in the model."""
+        return set([e for e in self.get_elements() if isinstance(e, Person)])
+
     @classmethod
     def hydrate(cls, model_io: ModelIO) -> "Model":
-        """"""
+        """Return a new model, hydrated from its IO."""
         model = cls(
             enterprise=Enterprise.hydrate(model_io.enterprise)
             if model_io.enterprise is not None
@@ -128,12 +136,10 @@ def hydrate(cls, model_io: ModelIO) -> "Model":
         )
 
         for person_io in model_io.people:
-            person = Person.hydrate(person_io)
-            model.add_person(person=person)
+            model += Person.hydrate(person_io)
 
         for software_system_io in model_io.software_systems:
-            software_system = SoftwareSystem.hydrate(software_system_io, model=model)
-            model.add_software_system(software_system=software_system)
+            model += SoftwareSystem.hydrate(software_system_io, model=model)
 
         # for deployment_node_io in model_io.deployment_nodes:
         #     deployment_node = DeploymentNode.hydrate(deployment_node_io)
@@ -154,12 +160,10 @@ def add_person(self, person=None, **kwargs) -> Person:
         Add a new person to the model.
 
         Args:
-            person (Person, optional): Either provide a `Person` instance or
-            **kwargs: Provide keyword arguments for instantiating a `Person`
-                (recommended).
+            **kwargs: Provide keyword arguments for instantiating a `Person`.
 
         Returns:
-            Person: Either the same or a new instance, depending on arguments.
+            Person: New instance.
 
         Raises:
             ValueError: When a person with the same name already exists.
@@ -168,28 +172,20 @@ def add_person(self, person=None, **kwargs) -> Person:
             Person
 
         """
-        if person is None:
-            person = Person(**kwargs)
-        if any(person.name == p.name for p in self.people):
-            ValueError(
-                f"A person with the name '{person.name}' already exists in the model."
-            )
-        self._add_element(person)
-        self.people.add(person)
+        person = Person(**kwargs)
+        self += person
         return person
 
-    def add_software_system(self, software_system=None, **kwargs) -> SoftwareSystem:
+    def add_software_system(self, **kwargs) -> SoftwareSystem:
         """
         Add a new software system to the model.
 
         Args:
-            software_system (SoftwareSystem, optional): Either provide a
-                `SoftwareSystem` instance or
             **kwargs: Provide keyword arguments for instantiating a `SoftwareSystem`
                 (recommended).
 
         Returns:
-            SoftwareSystem: Either the same or a new instance, depending on arguments.
+            SoftwareSystem: New instance.
 
         Raises:
             ValueError: When a software system with the same name already exists.
@@ -198,55 +194,32 @@ def add_software_system(self, software_system=None, **kwargs) -> SoftwareSystem:
             SoftwareSystem
 
         """
-        if software_system is None:
-            software_system = SoftwareSystem(**kwargs)
-        if any(software_system.name == s.name for s in self.software_systems):
-            ValueError(
-                f"A software system with the name {software_system.name} already "
-                f"exists in the model."
-            )
-        self._add_element(software_system)
-        self.software_systems.add(software_system)
+        software_system = SoftwareSystem(**kwargs)
+        self += software_system
         return software_system
 
-    def __iadd__(self, element: Union[Person, SoftwareSystem]) -> "Model":
-        """Add a new Person or SoftwareSystem to the model."""
+    def __iadd__(self, element: Element) -> "Model":
+        """Add a newly constructed element to the model."""
         if isinstance(element, Person):
-            self.add_person(person=element)
+            if any(element.name == p.name for p in self.people):
+                raise ValueError(
+                    f"A person with the name '{element.name}' already exists in the "
+                    f"model."
+                )
         elif isinstance(element, SoftwareSystem):
-            self.add_software_system(software_system=element)
-        else:
+            if any(element.name == s.name for s in self.software_systems):
+                raise ValueError(
+                    f"A software system with the name '{element.name}' already "
+                    f"exists in the model."
+                )
+        elif element.parent is None:
             raise ValueError(
-                f"Cannot add element with the name {element.name} to Model with += as it is not a Person or a SoftwareSystem."
+                f"Element with name {element.name} has no parent.  Please ensure "
+                f"you have added it to the parent element."
             )
+        self._add_element(element)
         return self
 
-    def add_container(self, container: Container) -> Container:
-        """
-        Register a newly constructed container with the model.
-
-        Args:
-            container (Container): `Container` instance to register.
-
-        Returns:
-            Container: The provided container.
-
-        Raises:
-            ValueError: When the container isn't a child of a `SoftwareSystem`
-
-        See Also:
-            Container
-
-        """
-        if container.parent is None:
-            raise ValueError(
-                f"Container with name {container.name} has no parent software system."
-                f"Adding to software system will register with the system's model."
-            )
-
-        self._add_element(container)
-        return container
-
     def add_container_instance(
         self,
         deployment_node: DeploymentNode,
@@ -275,20 +248,6 @@ def add_container_instance(
         # TODO: implement
         # instance_number =
 
-    def add_component(
-        self,
-        component: Component,
-    ) -> Component:
-        """Register a newly constructed Component with the model."""
-        if component.parent is None:
-            raise ValueError(
-                f"Component with name {component.name} has no parent container.  "
-                f"Adding to container will register with the container's model."
-            )
-
-        self._add_element(component)
-        return component
-
     def add_deployment_node(
         self, deployment_node: Optional[DeploymentNode] = None, **kwargs
     ) -> DeploymentNode:
@@ -383,16 +342,17 @@ def get_relationships(self) -> ValuesView[Relationship]:
         return self._relationships_by_id.values()
 
     def get_elements(self) -> ValuesView[Element]:
+        """Return an iterator over all elements contained in this model."""
         return self._elements_by_id.values()
 
     def get_software_system_with_id(self, id: str) -> Optional[SoftwareSystem]:
+        """Return the software system with a given ID."""
         result = self.get_element(id)
         if not isinstance(result, SoftwareSystem):
             return None
         return result
 
     def _add_element(self, element: Element) -> None:
-        """"""
         if not element.id:
             element.id = self._id_generator.generate_id()
         elif (
@@ -405,7 +365,6 @@ def _add_element(self, element: Element) -> None:
         self._id_generator.found(element.id)
 
     def _add_relationship(self, relationship: Relationship) -> bool:
-        """"""
         if relationship in self.get_relationships():
             return True
         if not relationship.id:

src/structurizr/model/person.py

@@ -16,9 +16,12 @@
 """Provide a person model."""
 
 
+from typing import Optional
+
 from pydantic import Field
 
 from .location import Location
+from .relationship import Relationship
 from .static_structure_element import StaticStructureElement, StaticStructureElementIO
 from .tags import Tags
 
@@ -50,19 +53,22 @@ class Person(StaticStructureElement):
     """
 
     def __init__(self, *, location: Location = Location.Unspecified, **kwargs) -> None:
-        """"""
+        """Initialise a Person."""
         super().__init__(**kwargs)
         self.location = location
 
         self.tags.add(Tags.PERSON)
 
     @classmethod
     def hydrate(cls, person_io: PersonIO) -> "Person":
-        """"""
+        """Create a new person and hydrate from its IO."""
         return cls(
             **cls.hydrate_arguments(person_io),
             location=person_io.location,
         )
 
-    def interacts_with(self, destination: "Person", description: str, **kwargs):
+    def interacts_with(
+        self, destination: "Person", description: str, **kwargs
+    ) -> Optional[Relationship]:
+        """Create a relationship with the given other Person."""
         return self.uses(destination=destination, description=description, **kwargs)

tests/integration/test_model_elements.py

@@ -48,7 +48,7 @@ def test_add_person_from_args(attributes: dict, model: Model):
 def test_add_person(attributes: dict, model: Model):
     """Expect that a person can be added to the model."""
     person = Person(**attributes)
-    model.add_person(person)
+    model += person
     assert person.id == "1"
     assert len(model.people) == 1
     for attr, expected in attributes.items():
@@ -75,7 +75,7 @@ def test_add_software_system_from_args(attributes: dict, model: Model):
 def test_add_software_system(attributes: dict, model: Model):
     """Expect that a software system can be added to the model."""
     software_system = SoftwareSystem(**attributes)
-    model.add_software_system(software_system)
+    model += software_system
     assert software_system.id == "1"
     assert len(model.software_systems) == 1
     for attr, expected in attributes.items():

tests/unit/model/test_container.py

@@ -22,12 +22,12 @@
 class MockModel:
     """Implement a mock model for testing."""
 
-    def add_component(self, component):
+    def __iadd__(self, component):
         """Simulate the model assigning IDs to new elements."""
         if not component.id:
             component.id = "id"
         component.set_model(self)
-        pass
+        return self
 
 
 _model = MockModel()

tests/unit/model/test_model.py

@@ -67,15 +67,15 @@ def test_model_cannot_add_relationship_with_same_id_as_element(empty_model: Mode
 def test_model_add_component_must_have_parent(empty_model: Model):
     """Ensure that Model rejects adding Components that aren't within a Container."""
     component = Component(name="c1")
-    with pytest.raises(ValueError, match="Component with name .* has no parent"):
-        empty_model.add_component(component)
+    with pytest.raises(ValueError, match="Element with name .* has no parent"):
+        empty_model += component
 
 
 def test_model_add_container_must_have_parent(empty_model: Model):
     """Ensure Model rejects adding Containers that aren't within a SoftwareSystem."""
     container = Container(name="c1")
-    with pytest.raises(ValueError, match="Container with name .* has no parent"):
-        empty_model.add_container(container)
+    with pytest.raises(ValueError, match="Element with name .* has no parent"):
+        empty_model += container
 
 
 def test_model_add_person_with_plusequals(empty_model: Model):
@@ -94,12 +94,31 @@ def test_model_add_software_system_with_plusequals(empty_model: Model):
     assert sys.id != ""
 
 
-def test_model_can_only_add_person_or_software_system_with_plusequals(
+def test_model_can_add_elements_with_plusequals(
     empty_model: Model,
 ):
-    """Ensure passing something other than a Person or SoftwareSystem in to += fails."""
+    """Ensure passing something other than a Person or SoftwareSystem to += works."""
+    sys = SoftwareSystem(name="Sys")
     c = Container(name="C")
+    c.parent = sys
+    empty_model += c
+    assert c in empty_model.get_elements()
+
+
+def test_model_cannot_add_two_people_with_same_name(empty_model: Model):
+    """Ensure duplicate people are not allowed."""
+    empty_model.add_person(name="Bob")
+    with pytest.raises(
+        ValueError, match="A person with the name 'Bob' already exists in the model."
+    ):
+        empty_model.add_person(name="Bob")
+
+
+def test_model_cannot_add_two_software_systems_with_same_name(empty_model: Model):
+    """Ensure duplicate software systems are not allowed."""
+    empty_model.add_software_system(name="Bob")
     with pytest.raises(
-        ValueError, match="Cannot add element with the name .* to Model"
+        ValueError,
+        match="A software system with the name 'Bob' already exists in the model.",
     ):
-        empty_model += c
+        empty_model.add_software_system(name="Bob")