Change Diff/DiffElement print_detailed() to str() and add test coverage

Author: glennmatthews

README.md

@@ -13,7 +13,7 @@ B.load()
 
 # it will show the difference between both systems
 diff_a_b = A.diff_from(B)
-diff.print_detailed()
+print(diff.str())
 
 # it will update System A to align with the current status of system B
 A.sync_from(B)

dsync/diff.py

@@ -16,7 +16,7 @@
 """
 
 from functools import total_ordering
-from typing import Any, Iterator, Iterable, Mapping, Optional
+from typing import Any, Iterator, Iterable, Mapping, Optional, Text
 
 from .exceptions import ObjectAlreadyExists
 from .utils import intersection, OrderedDefaultDict
@@ -64,7 +64,7 @@ def has_diffs(self) -> bool:
         """
         for group in self.groups():
             for child in self.children[group].values():
-                if child.has_diffs():
+                if child.has_diffs(include_children=True):
                     return True
 
         return False
@@ -95,26 +95,30 @@ def order_children_default(cls, children: dict) -> Iterator["DiffElement"]:
         for child in children.values():
             yield child
 
-    def print_detailed(self, indent: int = 0):
-        """Print all diffs to screen for all child elements.
-
-        Args:
-            indent (int, optional): Indentation to use when printing to screen. Defaults to 0.
-        """
+    def str(self, indent: int = 0):
+        """Build a detailed string representation of this Diff and its child DiffElements."""
         margin = " " * indent
+        output = []
         for group in self.groups():
-            print(f"{margin}{group}")
+            group_heading_added = False
             for child in self.children[group].values():
-                if child.has_diffs():
-                    child.print_detailed(indent + 2)
+                if child.has_diffs(include_children=True):
+                    if not group_heading_added:
+                        output.append(f"{margin}{group}")
+                        group_heading_added = True
+                    output.append(child.str(indent + 2))
+        result = "\n".join(output)
+        if not result:
+            result = "(no diffs)"
+        return result
 
 
 @total_ordering
 class DiffElement:  # pylint: disable=too-many-instance-attributes
     """DiffElement object, designed to represent a single item/object that may or may not have any diffs."""
 
     def __init__(
-        self, obj_type: str, name: str, keys: dict, source_name: str = "source", dest_name: str = "dest"
+        self, obj_type: Text, name: Text, keys: dict, source_name: Text = "source", dest_name: Text = "dest"
     ):  # pylint: disable=too-many-arguments
         """Instantiate a DiffElement.
 
@@ -170,7 +174,7 @@ def __str__(self):
         return f'{self.type} "{self.name}" : {self.keys} : {self.source_name} → {self.dest_name} : {self.get_attrs_diffs()}'
 
     @property
-    def action(self) -> Optional[str]:
+    def action(self) -> Optional[Text]:
         """Action, if any, that should be taken to remediate the diffs described by this element.
 
         Returns:
@@ -199,7 +203,7 @@ def add_attrs(self, source: Optional[dict] = None, dest: Optional[dict] = None):
         if dest is not None:
             self.dest_attrs = dest
 
-    def get_attrs_keys(self) -> Iterable[str]:
+    def get_attrs_keys(self) -> Iterable[Text]:
         """Get the list of shared attrs between source and dest, or the attrs of source or dest if only one is present.
 
         - If source_attrs is not set, return the keys of dest_attrs
@@ -215,8 +219,8 @@ def get_attrs_keys(self) -> Iterable[str]:
         return []
 
     # The below would be more accurate but typing.Literal is only in Python 3.8 and later
-    # def get_attrs_diffs(self) -> Mapping[str, Mapping[Literal["src", "dst"], Any]]:
-    def get_attrs_diffs(self) -> Mapping[str, Mapping[str, Any]]:
+    # def get_attrs_diffs(self) -> Mapping[Text, Mapping[Literal["src", "dst"], Any]]:
+    def get_attrs_diffs(self) -> Mapping[Text, Mapping[Text, Any]]:
         """Get the dict of actual attribute diffs between source_attrs and dest_attrs.
 
         Returns:
@@ -269,22 +273,25 @@ def has_diffs(self, include_children: bool = True) -> bool:
 
         return False
 
-    def print_detailed(self, indent: int = 0):
-        """Print status on screen for current object and all children.
-
-        Args:
-          indent: Default value = 0
-        """
+    def str(self, indent: int = 0):
+        """Build a detailed string representation of this DiffElement and its children."""
         margin = " " * indent
-
-        if self.source_attrs is None:
-            print(f"{margin}{self.type}: {self.name} MISSING in {self.source_name}")
-        elif self.dest_attrs is None:
-            print(f"{margin}{self.type}: {self.name} MISSING in {self.dest_name}")
-        else:
-            print(f"{margin}{self.type}: {self.name}")
+        result = f"{margin}{self.type}: {self.name}"
+        if self.source_attrs is not None and self.dest_attrs is not None:
             # Only print attrs that have meaning in both source and dest
             for attr, item in self.get_attrs_diffs().items():
-                print(f"{margin}  {attr}   {self.source_name}({item.get('src')})   {self.dest_name}({item.get('dst')})")
-
-        self.child_diff.print_detailed(indent + 2)
+                result += (
+                    f"\n{margin}  {attr}"
+                    f"    {self.source_name}({item.get('src')})"
+                    f"    {self.dest_name}({item.get('dst')})"
+                )
+        elif self.dest_attrs is not None:
+            result += f" MISSING in {self.source_name}"
+        elif self.source_attrs is not None:
+            result += f" MISSING in {self.dest_name}"
+
+        if self.child_diff.has_diffs():
+            result += "\n" + self.child_diff.str(indent + 2)
+        elif self.source_attrs is None and self.dest_attrs is None:
+            result += " (no diffs)"
+        return result

examples/example1/README.md

@@ -38,25 +38,25 @@ enable_console_logging(verbosity=0)  # Show WARNING and ERROR logs only
 Show the differences between A and B
 ```python
 diff_a_b = a.diff_to(b)
-diff_a_b.print_detailed()
+print(diff_a_b.str())
 ```
 
 Show the differences between B and C
 ```python
 diff_b_c = c.diff_from(b)
-diff_b_c.print_detailed()
+print(diff_b_c.str())
 ```
 
 Synchronize A and B (update B with the contents of A)
 ```python
 a.sync_to(b)
-a.diff_to(b).print_detailed()
+print(a.diff_to(b).str())
 ```
 
 Now A and B will show no differences
 ```python
 diff_a_b = a.diff_to(b)
-diff_a_b.print_detailed()
+print(diff_a_b.str())
 ```
 
 > In the Device model, the `site_name` and `role` are not included in the `_attributes`, so they are not shown when we are comparing the different objects, even if the value is different.

examples/example1/main.py

@@ -61,12 +61,12 @@ def main():
 
     print("Getting diffs from Backend A to Backend B...")
     diff_a_b = backend_a.diff_to(backend_b, diff_class=MyDiff)
-    diff_a_b.print_detailed()
+    print(diff_a_b.str())
 
     print("Syncing changes from Backend A to Backend B...")
     backend_a.sync_to(backend_b)
     print("Getting updated diffs from Backend A to Backend B...")
-    backend_a.diff_to(backend_b).print_detailed()
+    print(backend_a.diff_to(backend_b).str())
 
 
 if __name__ == "__main__":

tests/unit/test_diff.py

@@ -30,7 +30,11 @@ def test_diff_empty():
     assert not diff.has_diffs()
     assert list(diff.get_children()) == []
 
-    # TODO: test print_detailed
+
+def test_diff_str_with_no_diffs():
+    diff = Diff()
+
+    assert diff.str() == "(no diffs)"
 
 
 def test_diff_children():
@@ -63,7 +67,26 @@ def test_diff_children():
 
     assert diff.has_diffs()
 
-    # TODO: test print_detailed
+
+def test_diff_str_with_diffs():
+    diff = Diff()
+    device_element = DiffElement("device", "device1", {"name": "device1"})
+    diff.add(device_element)
+    intf_element = DiffElement("interface", "eth0", {"device_name": "device1", "name": "eth0"})
+    source_attrs = {"interface_type": "ethernet", "description": "my interface"}
+    dest_attrs = {"description": "your interface"}
+    intf_element.add_attrs(source=source_attrs, dest=dest_attrs)
+    diff.add(intf_element)
+
+    # Since device_element has no diffs, we don't have any "device" entry in the diff string:
+    assert (
+        diff.str()
+        == """\
+interface
+  interface: eth0
+    description    source(my interface)    dest(your interface)\
+"""
+    )
 
 
 def test_order_children_default(backend_a, backend_b):

tests/unit/test_diff_element.py

@@ -40,7 +40,11 @@ def test_diff_element_empty():
     )
     assert element2.source_name == "S1"
     assert element2.dest_name == "D1"
-    # TODO: test print_detailed
+
+
+def test_diff_element_str_with_no_diffs():
+    element = DiffElement("interface", "eth0", {"device_name": "device1", "name": "eth0"})
+    assert element.str() == "interface: eth0 (no diffs)"
 
 
 def test_diff_element_attrs():
@@ -66,7 +70,19 @@ def test_diff_element_attrs():
     assert element.has_diffs(include_children=False)
     assert element.get_attrs_keys() == ["description"]  # intersection of source_attrs.keys() and dest_attrs.keys()
 
-    # TODO: test print_detailed
+
+def test_diff_element_str_with_diffs():
+    element = DiffElement("interface", "eth0", {"device_name": "device1", "name": "eth0"})
+    element.add_attrs(source={"interface_type": "ethernet", "description": "my interface"})
+    assert element.str() == "interface: eth0 MISSING in dest"
+    element.add_attrs(dest={"description": "your interface"})
+    assert (
+        element.str()
+        == """\
+interface: eth0
+  description    source(my interface)    dest(your interface)\
+"""
+    )
 
 
 def test_diff_element_children():
@@ -88,4 +104,21 @@ def test_diff_element_children():
     assert parent_element.has_diffs(include_children=True)
     assert not parent_element.has_diffs(include_children=False)
 
-    # TODO: test print_detailed
+
+def test_diff_element_str_with_child_diffs():
+    child_element = DiffElement("interface", "eth0", {"device_name": "device1", "name": "eth0"})
+    parent_element = DiffElement("device", "device1", {"name": "device1"})
+    parent_element.add_child(child_element)
+    source_attrs = {"interface_type": "ethernet", "description": "my interface"}
+    dest_attrs = {"description": "your interface"}
+    child_element.add_attrs(source=source_attrs, dest=dest_attrs)
+
+    assert (
+        parent_element.str()
+        == """\
+device: device1
+  interface
+    interface: eth0
+      description    source(my interface)    dest(your interface)\
+"""
+    )

tests/unit/test_dsync.py

@@ -346,7 +346,7 @@ def test_dsync_sync_from_with_continue_on_failure_flag(log, error_prone_backend_
     error_prone_backend_a.sync_from(backend_b, flags=DSyncFlags.CONTINUE_ON_FAILURE)
     # Not all sync operations succeeded on the first try
     remaining_diffs = error_prone_backend_a.diff_from(backend_b)
-    remaining_diffs.print_detailed()
+    print(remaining_diffs.str())  # for debugging of any failure
     assert remaining_diffs.has_diffs()
 
     # At least some operations of each type should have succeeded
@@ -364,7 +364,7 @@ def test_dsync_sync_from_with_continue_on_failure_flag(log, error_prone_backend_
         print(f"Sync retry #{i}")
         error_prone_backend_a.sync_from(backend_b, flags=DSyncFlags.CONTINUE_ON_FAILURE)
         remaining_diffs = error_prone_backend_a.diff_from(backend_b)
-        remaining_diffs.print_detailed()
+        print(remaining_diffs.str())  # for debugging of any failure
         if remaining_diffs.has_diffs():
             # If we still have diffs, some ERROR messages should have been logged
             assert [event for event in log.events if event["level"] == "error"] != []
@@ -427,7 +427,7 @@ def test_dsync_diff_with_ignore_flag_on_source_models(backend_a, backend_a_with_
     backend_a_with_extra_models.get(backend_a_with_extra_models.site, "nyc").model_flags |= DSyncModelFlags.IGNORE
 
     diff = backend_a.diff_from(backend_a_with_extra_models)
-    diff.print_detailed()
+    print(diff.str())  # for debugging of any failure
     assert not diff.has_diffs()
 
 
@@ -438,7 +438,7 @@ def test_dsync_diff_with_ignore_flag_on_target_models(backend_a, backend_a_minus
     backend_a.get(backend_a.site, "sfo").model_flags |= DSyncModelFlags.IGNORE
 
     diff = backend_a.diff_from(backend_a_minus_some_models)
-    diff.print_detailed()
+    print(diff.str())  # for debugging of any failure
     assert not diff.has_diffs()
 
 
@@ -471,5 +471,5 @@ class NoDeleteInterfaceDSync(BackendA):
     assert extra_models.get(extra_models.interface, extra_interface.get_unique_id()) is None
     # The sync should be complete, regardless
     diff = extra_models.diff_from(backend_a)
-    diff.print_detailed()
+    print(diff.str())  # for debugging of any failure
     assert not diff.has_diffs()