From 4ee5ffd0871fcab8e814c0eaade0f184570269b1 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Thu, 14 May 2015 13:04:24 -0700 Subject: [PATCH] Add a new `ls_r` method Instead of having a `ls` method that when used recursively *always* returns the absolute path of items in the fake in memory storage tree and *relative* paths (when used in non-recursive mode) add a new `ls_r` method that can return absolute *or* relative paths. In the future it is highly likely that the the `ls` recursive keyword argument will be removed (so preferring and moving to the `ls_r` should occur earlier rather than later), so this also adds a debtcollector removed keyword argument decorator over the existing `ls` to ensure that users are aware of this change (as well as a adjusted docstring). Fixes bug 1458114 Change-Id: Id2a5869e94ac44679020a14297d1073d1dc2718f --- taskflow/examples/dump_memory_backend.py | 2 +- taskflow/persistence/backends/impl_memory.py | 63 +++++++++++++++++-- .../persistence/test_memory_persistence.py | 60 +++++++++++++++++- 3 files changed, 117 insertions(+), 8 deletions(-) diff --git a/taskflow/examples/dump_memory_backend.py b/taskflow/examples/dump_memory_backend.py index 2e3aee71..6c6d5488 100644 --- a/taskflow/examples/dump_memory_backend.py +++ b/taskflow/examples/dump_memory_backend.py @@ -70,7 +70,7 @@ e.run() print("---------") print("After run") print("---------") -for path in backend.memory.ls(backend.memory.root_path, recursive=True): +for path in backend.memory.ls_r(backend.memory.root_path, absolute=True): value = backend.memory[path] if value: print("%s -> %s" % (path, value)) diff --git a/taskflow/persistence/backends/impl_memory.py b/taskflow/persistence/backends/impl_memory.py index 0aac58eb..43207b81 100644 --- a/taskflow/persistence/backends/impl_memory.py +++ b/taskflow/persistence/backends/impl_memory.py @@ -20,6 +20,7 @@ import copy import itertools import posixpath as pp +from debtcollector import removals import fasteners import six @@ -166,13 +167,63 @@ class FakeFilesystem(object): else: return self._copier(node.metadata['value']) - def ls(self, path, recursive=False): - """Return list of all children of the given path.""" - if not recursive: - return [node.item for node in self._fetch_node(path)] + def _up_to_root_selector(self, root_node, child_node): + # Build the path from the child to the root and stop at the + # root, and then form a path string... + path_pieces = [child_node.item] + for parent_node in child_node.path_iter(include_self=False): + if parent_node is root_node: + break + path_pieces.append(parent_node.item) + if len(path_pieces) > 1: + path_pieces.reverse() + return self.join(*path_pieces) + + @staticmethod + def _metadata_path_selector(root_node, child_node): + return child_node.metadata['path'] + + def ls_r(self, path, absolute=False): + """Return list of all children of the given path (recursively).""" + node = self._fetch_node(path) + if absolute: + selector_func = self._metadata_path_selector else: - node = self._fetch_node(path) - return [child.metadata['path'] for child in node.bfs_iter()] + selector_func = self._up_to_root_selector + return [selector_func(node, child_node) + for child_node in node.bfs_iter()] + + @removals.removed_kwarg('recursive', version="0.11", removal_version="?") + def ls(self, path, recursive=False): + """Return list of all children of the given path. + + NOTE(harlowja): if ``recursive`` is passed in as truthy then the + absolute path is **always** returned (not the relative path). If + ``recursive`` is left as the default or falsey then the + relative path is **always** returned. + + This is documented in bug `1458114`_ and the existing behavior is + being maintained, to get a recursive version that is absolute (or is + not absolute) it is recommended to use the :py:meth:`.ls_r` method + instead. + + .. deprecated:: 0.11 + + In a future release the ``recursive`` keyword argument will + be removed (so preferring and moving to the :py:meth:`.ls_r` should + occur earlier rather than later). + + .. _1458114: https://bugs.launchpad.net/taskflow/+bug/1458114 + """ + node = self._fetch_node(path) + if recursive: + selector_func = self._metadata_path_selector + child_node_it = node.bfs_iter() + else: + selector_func = self._up_to_root_selector + child_node_it = iter(node) + return [selector_func(node, child_node) + for child_node in child_node_it] def clear(self): """Remove all nodes (except the root) from this filesystem.""" diff --git a/taskflow/tests/unit/persistence/test_memory_persistence.py b/taskflow/tests/unit/persistence/test_memory_persistence.py index 24f76aa3..80686396 100644 --- a/taskflow/tests/unit/persistence/test_memory_persistence.py +++ b/taskflow/tests/unit/persistence/test_memory_persistence.py @@ -71,7 +71,7 @@ class MemoryFilesystemTest(test.TestCase): self.assertEqual('c', fs['/c']) self.assertEqual('db', fs['/d/b']) - def test_ls_recursive(self): + def test_old_ls_recursive(self): fs = impl_memory.FakeFilesystem() fs.ensure_path("/d") fs.ensure_path("/c/d") @@ -91,7 +91,65 @@ class MemoryFilesystemTest(test.TestCase): '/a/b/c/d', ], contents) + def test_ls_recursive(self): + fs = impl_memory.FakeFilesystem() + fs.ensure_path("/d") + fs.ensure_path("/c/d") + fs.ensure_path("/b/c/d") + fs.ensure_path("/a/b/c/d") + contents = fs.ls_r("/", absolute=False) + self.assertEqual([ + 'a', + 'b', + 'c', + 'd', + 'a/b', + 'b/c', + 'c/d', + 'a/b/c', + 'b/c/d', + 'a/b/c/d', + ], contents) + + def test_ls_recursive_absolute(self): + fs = impl_memory.FakeFilesystem() + fs.ensure_path("/d") + fs.ensure_path("/c/d") + fs.ensure_path("/b/c/d") + fs.ensure_path("/a/b/c/d") + contents = fs.ls_r("/", absolute=True) + self.assertEqual([ + '/a', + '/b', + '/c', + '/d', + '/a/b', + '/b/c', + '/c/d', + '/a/b/c', + '/b/c/d', + '/a/b/c/d', + ], contents) + def test_ls_recursive_targeted(self): + fs = impl_memory.FakeFilesystem() + fs.ensure_path("/d") + fs.ensure_path("/c/d") + fs.ensure_path("/b/c/d") + fs.ensure_path("/a/b/c/d") + contents = fs.ls_r("/a/b", absolute=False) + self.assertEqual(['c', 'c/d'], contents) + + def test_ls_recursive_targeted_absolute(self): + fs = impl_memory.FakeFilesystem() + fs.ensure_path("/d") + fs.ensure_path("/c/d") + fs.ensure_path("/b/c/d") + fs.ensure_path("/a/b/c/d") + contents = fs.ls_r("/a/b", absolute=True) + self.assertEqual(['/a/b/c', '/a/b/c/d'], contents) + + def test_old_ls_recursive_targeted_absolute(self): fs = impl_memory.FakeFilesystem() fs.ensure_path("/d") fs.ensure_path("/c/d")