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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

Fixes #890.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the listdir_rmdir branch 2 times, most recently from 4010b32 to 6be2a17 Compare December 2, 2021 09:40
@codecov
Copy link

codecov bot commented Dec 2, 2021 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.94%. Comparing base (f0677c2) to head (86ea815).
Report is 465 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #903   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          32       32           
  Lines       11218    11218           
=======================================
  Hits        11212    11212           
  Misses          6        6
Files with missing lines Coverage Δ
zarr/_storage/store.py 100.00% <100.00%> (ø)
zarr/storage.py 100.00% <100.00%> (ø)

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft December 2, 2021 09:51
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the listdir_rmdir branch 2 times, most recently from 6dc45a5 to a48da3b Compare December 2, 2021 10:11
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Dec 2, 2021 •
edited
Loading

It works by changing the type of the path argument from Path = Union[str, bytes, None] to str everywhere. I suspect bytes is obsolete/not required, and I believe None can be implicit.

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review December 2, 2021 10:32
joshmoore
joshmoore reviewed Dec 2, 2021
View reviewed changes
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

Few thoughts, but this one may need a few more eyes on it.

zarr/_storage/store.py
@@ -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

zarr/storage.py
@@ -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 •
edited
Loading

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.

@sanketverma1704 sanketverma1704 mentioned this pull request Dec 20, 2022
Open
@jhamman
Copy link
Member

jhamman commented Oct 11, 2024

I'm going to close this as stale. Folks should feel free to reopen if there is interest in continuing this work but understand that the Store API has changed significantly in v3.

@jhamman jhamman closed this Oct 11, 2024
@DimitriPapadopoulos DimitriPapadopoulos deleted the listdir_rmdir branch October 12, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshmoore joshmoore joshmoore left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent listdir() and rmdir() method signature across class hierarchy
3 participants
@DimitriPapadopoulos @jhamman @joshmoore