Repository: PyFilesystem/pyfilesystem2
Pull request #119 (2017-12-18 19:37:50)

walker_class attribute for filesystem
I needed to write custom walker for my custom fs. It was done via override of 'walk' property. Everything was fine until I opened subdir. It turned out that WrapFS inherits 'walk' property from base FS, which always uses default Walker class. 
I propose introduction of 'walker_class' fs class-level attribute which is used in 'walk' propery instead of hard-coded Walker class. With this, custom fs will only need to redefine it to use another walker.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14709768/badge)](https://coveralls.io/builds/14709768)

Coverage decreased (-0.8%) to 99.116% when pulling **e101a7f7b410d149bc4c35472aa0e4370f82a8f8 on fresheed:walker_class** into **ee3e3353bf3c47b15c1afe00e378f14be47d7b60 on PyFilesystem:master**.

althonos: For any filesystem, the walk property is intended to be a shortcut, but when you want to have a custom walker, you should:

* declare a `CustomWalker` inheriting from `fs.walk.Walker`
* instantiate a `BoundWalker` instance that uses your `CustomWalker`
* use your `BoundWalker` as you would use a walker (or `fs.walk`)


For instance:
1. create a `NoHiddenWalker` that scans directories only if they are not hidden (in Unix-style):
  ```python
   import fs.walk
   class NoHiddenWalker(fs.walk.Walker):
      def check_scan_dir(self, fs, path, info):
         return not info.name.startswith('.')
   ```
2. Open your filesystem and obtain a `BoundWalker`:
   ```python
   my_fs = fs.open_fs("~")   
   my_walker = Boundwalker(my_fs, walker_class=NoHiddenWalker)
   ```
3. Use your walker normally:
   ```python
   for x in my_walker.files():
      # ... #
   ```
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14710294/badge)](https://coveralls.io/builds/14710294)

Coverage increased (+4.0e-05%) to 99.951% when pulling **785795b91faedab0317cee2f95ce035aff78bcfa on fresheed:walker_class** into **ee3e3353bf3c47b15c1afe00e378f14be47d7b60 on PyFilesystem:master**.

fresheed: Yes, we can instantiate concrete Walker directly, but for me one of most powerful features of pyfilesystem is consistent interface for all fs implementations. Direct creation of walkers brings unwanted coupling with concrete fs implementation. For example, I use custom walker for SVN traverse, and this walker is useless with another fs, so it would be nice to hide it under 'walk' attribute. Then all client modules just access 'walk' without need to know which fs/walker is used.
willmcgugan: @fresheed Could you expand on your use case? What does your custom walker do differently?
althonos: My problem with this is that it is the filesystem role to handle the logic about what is a file and what is not, the walker should only be there to iteratively traverse the entries the filesystem exposes...
willmcgugan: @althonos Yeah, I'm thinking the same. The `walk` attribute was only meant as a convenient shortcut. But maybe @fresheed has a use-case I hadn't considered.
fresheed: My use-case is working with SVN filesystem. Default walker calls listdir() recursively, and this process can take a lot of time. But SVN has method for full recursive listing which is performed server-side so it is quite fast. So my implementation of walker just calls this method and creates walk() output of retrieved entries.
willmcgugan: Ah, I see. So your custom walker is an optimization so you can use the SVN db.

@althonos I think that may be a legitimate reason to have a customizable walker class with the `walk` attribute. I don't think I'd go as far as recommending it as a general purpose mechanism, but its probably fine as a 'use at your own risk' deal. What do you think?
pombredanne: Another approach would be to allow FS to override walk? The use of a property there robs us from the ability to customize this directly?
althonos: @pombredanne : not at all, [a property can be overriden in a subclass as well](https://www.safaribooksonline.com/library/view/python-cookbook-3rd/9781449357337/ch08s08.html)

@willmcgugan : the same logic could probably be implemented as a filesystem or a filesystem wrapper, but I do agree @fresheed's usecase is actually better handled as a custom walker. In that case, I do agree that having a `walker_class` attribute would make the best-performing walker always available as the default.
willmcgugan: > There is also another possibility: it could be possible to have WrapFS.walk use the same walker class as the delegate filesystem.

I think @fresheed 's change also does that. There is a modification in wrapfs which gets the walker from the delegated filesystem.

I think I'm going to go with this one. At some point I'll document the pros and cons.

Thanks @fresheed 


Patch:
diff --git a/fs/base.py b/fs/base.py
index b0ae0b8..4672161 100644
--- a/fs/base.py
+++ b/fs/base.py
@@ -43,6 +43,9 @@ class FS(object):
 
     # This is the "standard" meta namespace.
     _meta = {}
+    
+    # most FS will use default walking algorithms
+    walker_class=Walker
 
     def __init__(self):
         """Create a filesystem. See help(type(self)) for accurate signature.
@@ -65,7 +68,7 @@ class FS(object):
     def walk(self):
         """`~fs.walk.BoundWalker`: a walker bound to this filesystem.
         """
-        return Walker.bind(self)
+        return self.walker_class.bind(self)
 
     # ---------------------------------------------------------------- #
     # Required methods                                                 #
diff --git a/fs/wrapfs.py b/fs/wrapfs.py
index f0db278..4ad396a 100644
--- a/fs/wrapfs.py
+++ b/fs/wrapfs.py
@@ -397,3 +397,8 @@ class WrapFS(FS):
             _fs.validatepath(_path)
         path = abspath(normpath(path))
         return path
+
+    @property
+    def walk(self):
+        return self._wrap_fs.walker_class.bind(self)
+


Test patch:
diff --git a/tests/test_walk.py b/tests/test_walk.py
index f6ac58b..fafee98 100644
--- a/tests/test_walk.py
+++ b/tests/test_walk.py
@@ -5,6 +5,8 @@ import unittest
 from fs.errors import FSError
 from fs.memoryfs import MemoryFS
 from fs import walk
+from fs.wrap import read_only
+import six
 
 
 class TestWalkerBase(unittest.TestCase):
@@ -196,3 +198,46 @@ class TestWalk(unittest.TestCase):
     def test_on_error_invalid(self):
         with self.assertRaises(TypeError):
             walk.Walker(on_error='nope')
+    
+    def test_subdir_uses_same_walker(self):
+        class CustomWalker(walk.Walker):
+
+            @classmethod
+            def bind(cls, fs):
+                return walk.BoundWalker(fs, walker_class=CustomWalker)
+
+        class CustomizedMemoryFS(MemoryFS):
+            walker_class=CustomWalker
+
+        base_fs=CustomizedMemoryFS()
+        base_fs.settext("a", "a")
+        base_fs.makedirs("b")
+        base_fs.settext("b/c", "c")
+        base_fs.settext("b/d", "d")
+        base_walker=base_fs.walk
+        self.assertEqual(base_walker.walker_class, CustomWalker)
+        six.assertCountEqual(self, ["/a", "/b/c", "/b/d"], base_walker.files())
+
+        sub_fs=base_fs.opendir("b")
+        sub_walker=sub_fs.walk
+        self.assertEqual(sub_walker.walker_class, CustomWalker)
+        six.assertCountEqual(self, ["/c", "/d"], sub_walker.files())
+
+    def test_readonly_wrapper_uses_same_walker(self):
+        class CustomWalker(walk.Walker):
+
+            @classmethod
+            def bind(cls, fs):
+                return walk.BoundWalker(fs, walker_class=CustomWalker)
+
+        class CustomizedMemoryFS(MemoryFS):
+            walker_class=CustomWalker
+
+        base_fs=CustomizedMemoryFS()
+        base_walker=base_fs.walk
+        self.assertEqual(base_walker.walker_class, CustomWalker)
+
+        readonly_fs=read_only(CustomizedMemoryFS())
+        readonly_walker=readonly_fs.walk
+        self.assertEqual(readonly_walker.walker_class, CustomWalker)
+