_yaml: Remove 'node_find_target' and replace by 'MappingNode.find'bschubert/node-find-target

4 files changed, 70 insertions, 68 deletions

diff --git a/src/buildstream/_yaml.pxd b/src/buildstream/_yaml.pxd
index 6e4fd6218..48bed1dea 100644
--- a/src/buildstream/_yaml.pxd
+++ b/src/buildstream/_yaml.pxd
@@ -29,6 +29,9 @@ cdef class Node:
 
     cpdef Node copy(self)
 
+    cdef bint _shares_position_with(self, Node target)
+    cdef bint _walk_find(self, Node target, list path) except *
+
 
 cdef class MappingNode(Node):
     cdef Node get(self, str key, default, default_constructor)
@@ -44,6 +47,8 @@ cdef class MappingNode(Node):
     cpdef void safe_del(self, str key)
     cpdef object values(self)
 
+    cpdef list _find(self, Node target)
+
 
 cdef class ScalarNode(Node):
     cpdef bint as_bool(self) except *
diff --git a/src/buildstream/_yaml.pyx b/src/buildstream/_yaml.pyx
index ef1a7f015..46f642578 100644
--- a/src/buildstream/_yaml.pyx
+++ b/src/buildstream/_yaml.pyx
@@ -68,6 +68,12 @@ cdef class Node:
         self.line = line
         self.column = column
 
+    cdef bint _walk_find(self, Node target, list path) except *:
+        raise NotImplementedError()
+
+    cdef bint _shares_position_with(self, Node target):
+        return self.file_index == target.file_index and self.line == target.line and self.column == target.column
+
     def __contains__(self, what):
         # Delegate to the inner value, though this will likely not work
         # very well if the node is a list or string, it's unlikely that
@@ -105,7 +111,7 @@ cdef class ScalarNode(Node):
             return False
         else:
             provenance = node_get_provenance(self)
-            path = node_find_target(provenance.toplevel, self)[-1]
+            path = provenance.toplevel._find(self)[-1]
             raise LoadError(LoadErrorReason.INVALID_DATA,
                 "{}: Value of '{}' is not of the expected type '{}'"
                 .format(provenance, path, bool.__name__, self.value))
@@ -115,7 +121,7 @@ cdef class ScalarNode(Node):
             return int(self.value)
         except ValueError:
             provenance = node_get_provenance(self)
-            path = node_find_target(provenance.toplevel, self)[-1]
+            path = provenance.toplevel._find(self)[-1]
             raise LoadError(LoadErrorReason.INVALID_DATA,
                 "{}: Value of '{}' is not of the expected type '{}'"
                 .format(provenance, path, int.__name__))
@@ -126,6 +132,9 @@ cdef class ScalarNode(Node):
             return None
         return str(self.value)
 
+    cdef bint _walk_find(self, Node target, list path) except *:
+        return self._shares_position_with(target)
+
 
 cdef class MappingNode(Node):
 
@@ -145,6 +154,23 @@ cdef class MappingNode(Node):
 
         return MappingNode(copy, self.file_index, self.line, self.column)
 
+    # find()
+    #
+    # Searches the given node tree for the given target node.
+    #
+    # This is typically used when trying to walk a path to a given node
+    # for the purpose of then modifying a similar tree of objects elsewhere
+    #
+    # Args:
+    #    target (Node): The node you are looking for in that tree
+    #
+    # Returns:
+    #    (list): A path from `node` to `target` or None if `target` is not in the subtree
+    cpdef list _find(self, Node target):
+        cdef list path = []
+        if self._walk_find(target, path):
+            return path
+        return None
 
     cdef Node get(self, str key, object default, object default_constructor):
         value = self.value.get(key, _sentinel)
@@ -280,6 +306,22 @@ cdef class MappingNode(Node):
 
             self.value[key] = node
 
+    cdef bint _walk_find(self, Node target, list path) except *:
+        cdef str k
+        cdef Node v
+
+        if self._shares_position_with(target):
+            return True
+
+        for k, v in self.value.items():
+            path.append(k)
+            if v._walk_find(target, path):
+                return True
+            del path[-1]
+
+        return False
+
+
 
 cdef class SequenceNode(Node):
     def __init__(self, list value, int file_index, int line, int column):
@@ -302,7 +344,7 @@ cdef class SequenceNode(Node):
 
         if type(value) is not MappingNode:
             provenance = node_get_provenance(self)
-            path = ["[{}]".format(p) for p in node_find_target(provenance.toplevel, self)] + ["[{}]".format(index)]
+            path = ["[{}]".format(p) for p in provenance.toplevel._find(self)] + ["[{}]".format(index)]
             raise LoadError(LoadErrorReason.INVALID_DATA,
                             "{}: Value of '{}' is not of the expected type '{}'"
                             .format(provenance, path, MappingNode.__name__))
@@ -313,7 +355,7 @@ cdef class SequenceNode(Node):
 
         if type(value) is not SequenceNode:
             provenance = node_get_provenance(self)
-            path = ["[{}]".format(p) for p in node_find_target(provenance.toplevel, self)] + ["[{}]".format(index)]
+            path = ["[{}]".format(p) for p in provenance.toplevel._find(self)] + ["[{}]".format(index)]
             raise LoadError(LoadErrorReason.INVALID_DATA,
                             "{}: Value of '{}' is not of the expected type '{}'"
                             .format(provenance, path, SequenceNode.__name__))
@@ -323,6 +365,21 @@ cdef class SequenceNode(Node):
     cpdef list as_str_list(self):
         return [node.as_str() for node in self.value]
 
+    cdef bint _walk_find(self, Node target, list path) except *:
+        cdef int i
+        cdef Node v
+
+        if self._shares_position_with(target):
+            return True
+
+        for i, v in enumerate(self.value):
+            path.append(i)
+            if v._walk_find(target, path):
+                return True
+            del path[-1]
+
+        return False
+
     def __iter__(self):
         return iter(self.value)
 
@@ -1329,66 +1386,6 @@ def assert_symbol_name(ProvenanceInformation provenance, str symbol_name, str pu
                         message, detail=detail)
 
 
-# node_find_target()
-#
-# Searches the given node tree for the given target node.
-#
-# This is typically used when trying to walk a path to a given node
-# for the purpose of then modifying a similar tree of objects elsewhere
-#
-# If the key is provided, then we actually hunt for the node represented by
-# target[key] and return its container, rather than hunting for target directly
-#
-# Args:
-#    node (Node): The node at the root of the tree to search
-#    target (Node): The node you are looking for in that tree
-#
-# Returns:
-#    (list): A path from `node` to `target` or None if `target` is not in the subtree
-cpdef list node_find_target(MappingNode node, Node target):
-    cdef list path = []
-    if _walk_find_target(node, path, target):
-        return path
-    return None
-
-
-# Helper for node_find_target() which walks a value
-cdef bint _walk_find_target(Node node, list path, Node target) except *:
-    if node.file_index == target.file_index and node.line == target.line and node.column == target.column:
-        return True
-    elif type(node.value) is dict:
-        return _walk_dict_node(node, path, target)
-    elif type(node.value) is list:
-        return _walk_list_node(node, path, target)
-    return False
-
-
-# Helper for node_find_target() which walks a list
-cdef bint _walk_list_node(Node node, list path, Node target):
-    cdef int i
-    cdef Node v
-
-    for i, v in enumerate(node.value):
-        path.append(i)
-        if _walk_find_target(v, path, target):
-            return True
-        del path[-1]
-    return False
-
-
-# Helper for node_find_target() which walks a mapping
-cdef bint _walk_dict_node(MappingNode node, list path, Node target):
-    cdef str k
-    cdef Node v
-
-    for k, v in node.value.items():
-        path.append(k)
-        if _walk_find_target(v, path, target):
-            return True
-        del path[-1]
-    return False
-
-
 ###############################################################################
 
 # Roundtrip code
diff --git a/src/buildstream/source.py b/src/buildstream/source.py
index 2274f47d9..c87d77020 100644
--- a/src/buildstream/source.py
+++ b/src/buildstream/source.py
@@ -968,9 +968,9 @@ class Source(Plugin):
 
             # Get the path to whatever changed
             if action == 'add':
-                path = _yaml.node_find_target(toplevel_node, node)
+                path = toplevel_node._find(node)
             else:
-                full_path = _yaml.node_find_target(toplevel_node, node.get_node(key))
+                full_path = toplevel_node._find(node.get_node(key))
                 # We want the path to the node containing the key, not to the key
                 path = full_path[:-1]
 
diff --git a/tests/internals/yaml.py b/tests/internals/yaml.py
index 1857a1b76..90f2a39f8 100644
--- a/tests/internals/yaml.py
+++ b/tests/internals/yaml.py
@@ -523,7 +523,7 @@ def test_node_find_target(datafiles, case):
             return node.value[entry]
 
     want = _walk(loaded, case[0], case[1:])
-    found_path = _yaml.node_find_target(toplevel, want)
+    found_path = toplevel._find(want)
 
     assert case == found_path
 
@@ -537,4 +537,4 @@ def test_node_find_target_fails(datafiles):
 
     brand_new = _yaml.new_empty_node()
 
-    assert _yaml.node_find_target(loaded, brand_new) is None
+    assert loaded._find(brand_new) is None