Add walk to epath.Path

[chicham]

Hi,

I also needed the walk method for a project and took the liberty to fix #520

Best,
Hicham

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[Conchylicultor]

Thank you for starting this! Mostly minor comments

[Conchylicultor]

Use the official pathlib.pyi signature and correct type annotations from: https://github.com/python/typeshed/blob/main/stdlib/pathlib.pyi

  @abstractmethod
  def walk(
      self,
      top_down: bool = True,
      on_error: Callable[[OSError], object] | None = None,
      follow_symlinks: bool = False,
  ) -> Iterator[tuple[Self, list[str], list[str]]]:

max_depth is never used, as well as **kwargs, so those should be removed everywhere.

Currently calling with a typo path.walk(top_dow=False) will silently bug.

[chicham]

The fsspec backend accepts a string as argument for on_error which is not supported by the official pathlib.pyi signature. The fsspec backend also introduces the kwargs and max_depth arguments. We can cover these cases as follow

  @abstractmethod
  def walk(
      self,
      top_down: bool = True,
      on_error: Callable[[OSError], object] | str | None = None,
      follow_symlinks: bool = False,
      max_depth: int | None = None,
      **kwargs: Any,
  ) -> Iterator[tuple[Self, list[str], list[str]]]:

To prevent the typos actually we can replace kwargs by an optional dict argument called fsspec_kwargs and use this dict only in the fsspec backend or we can simply remove kwargs but it will means to loose the ability to control how fsspec list files during the walk. what do you think about it ?

Notes:

• I modify the maxdepth args from fsspec to max_depth to make it easier to remember for the users.
• fsspec.walk doc https://filesystem-spec.readthedocs.io/en/latest/api.html?highlight=walk#fsspec.spec.AbstractFileSystem.walk

[Conchylicultor]

epath.Path is supposed to abstract away the backends and filesystem that the path use. The fact we use fsspec/tensorflow or other is an internal detail that might change in the future.

So I don't think the public API should expose any of max_depth and kwargs. Otherwise the same code would behave inconsistently across gcs / locally depending on which backend is used.

The fact not all options from all backend are exposed is not a problem. If max_depth is really required/requested/useful, it should be added first in pathlib.Path

[chicham]

done

[Conchylicultor]

Type annotations should be Iterator[tuple[Self, list[str], list[str]]]: and Callable[[OSError], object] | None

Same everywhere

[chicham]

done

[Conchylicultor]

Remove non-pathlib kwargs

[chicham]

done

[Conchylicultor]

Same

[chicham]

done

[Conchylicultor]

Behavior should match pathlib.Path.walk() which returns tuple[Self, list[str], list[str]

Only root is a pathlib-like, dir and files are list[str]

[chicham]

done

[Conchylicultor]

Rather than many conditions, How about testing

assert list(backend.walk(tmp_path)) == [
    (tmp_path, ['abc'], []),
    ...,
]

[chicham]

Good suggestions ! I updated the tests

[Conchylicultor]

Please also update the CHANGELOG.md file

[chicham]

done

[chicham]

@Conchylicultor I have one last question. The argument follow_symlinks of walk is only used by the os.path backend. Either fsspec and tf.io.gfile seem to ignore it.
Do you think we should raise a warning if this argument is set to True for the backends which do not support it ?

[Conchylicultor]

Thank you for the update. I imported this internally and am waiting for internal review to merge this.