Skip to content

Consistent signatures for listdir() and rmdir() #903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions zarr/_storage/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,11 @@ class Store(BaseStore):

"""

def listdir(self, path: str = "") -> List[str]:
def listdir(self, path: str = None) -> List[str]:
path = normalize_storage_path(path)
return _listdir_from_keys(self, path)

def rmdir(self, path: str = "") -> None:
def rmdir(self, path: str = None) -> None:
if not self.is_erasable():
raise NotImplementedError(
f'{type(self)} is not erasable, cannot call "rmdir"'
Expand Down Expand Up @@ -150,14 +150,6 @@ def _rename_from_keys(store: BaseStore, src_path: str, dst_path: str) -> None:
store[new_key] = store.pop(key)


def _rmdir_from_keys(store: Union[BaseStore, MutableMapping], path: Optional[str] = None) -> None:
# assume path already normalized
prefix = _path_to_prefix(path)
for key in list(store.keys()):
if key.startswith(prefix):
del store[key]


def _listdir_from_keys(store: BaseStore, path: Optional[str] = None) -> List[str]:
# assume path already normalized
prefix = _path_to_prefix(path)
Expand All @@ -168,3 +160,11 @@ def _listdir_from_keys(store: BaseStore, path: Optional[str] = None) -> List[str
child = suffix.split('/')[0]
children.add(child)
return sorted(children)


def _rmdir_from_keys(store: Union[BaseStore, MutableMapping], path: Optional[str] = None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this need to shift down?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just the order used everywhere else in the code: 1) listdir 2) rmdir

# assume path already normalized
prefix = _path_to_prefix(path)
for key in list(store.keys()):
if key.startswith(prefix):
del store[key]
12 changes: 6 additions & 6 deletions zarr/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ def normalize_store_arg(store: Any, clobber=False, storage_options=None, mode="w
return store


def rmdir(store: StoreLike, path: Path = None):
def rmdir(store: StoreLike, path: str = None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need some comparison to #768

Copy link
Contributor Author

@DimitriPapadopoulos DimitriPapadopoulos Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at #768:

  • I understand it would be nice if path could be PathLike in addition to str, wouldn't it?
  • BTW, I would rename _might_close() to _is_path_like(), it's easier to understand because that's really what the function is about.

"""Remove all items under the given path. If `store` provides a `rmdir` method,
this will be called, otherwise will fall back to implementation via the
`Store` interface."""
Expand All @@ -155,7 +155,7 @@ def rename(store: BaseStore, src_path: Path, dst_path: Path):
_rename_from_keys(store, src_path, dst_path)


def listdir(store: BaseStore, path: Path = None):
def listdir(store: BaseStore, path: str = None):
"""Obtain a directory listing for the given path. If `store` provides a `listdir`
method, this will be called, otherwise will fall back to implementation via the
`MutableMapping` interface."""
Expand Down Expand Up @@ -694,7 +694,7 @@ def __iter__(self):
def __len__(self) -> int:
return sum(1 for _ in self.keys())

def listdir(self, path: Path = None) -> List[str]:
def listdir(self, path: str = None) -> List[str]:
path = normalize_storage_path(path)
if path:
try:
Expand All @@ -718,7 +718,7 @@ def rename(self, src_path: Path, dst_path: Path):

dst_parent[dst_key] = src_parent.pop(src_key)

def rmdir(self, path: Path = None):
def rmdir(self, path: str = None):
path = normalize_storage_path(path)
if path:
try:
Expand Down Expand Up @@ -2157,7 +2157,7 @@ def _keys(self):
self._keys_cache = list(self._store.keys())
return self._keys_cache

def listdir(self, path: Path = None):
def listdir(self, path: str = None) -> List[str]:
with self._mutex:
try:
return self._listdir_cache[path]
Expand Down Expand Up @@ -2682,5 +2682,5 @@ def __setitem__(self, key, value):
def getsize(self, path):
return getsize(self.meta_store, path)

def listdir(self, path):
def listdir(self, path=None):
return listdir(self.meta_store, path)