Skip to content
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

Inconsistent listdir() and rmdir() method signature across class hierarchy #890

Open
DimitriPapadopoulos opened this issue Nov 26, 2021 · 4 comments · May be fixed by #903
Open

Inconsistent listdir() and rmdir() method signature across class hierarchy #890

DimitriPapadopoulos opened this issue Nov 26, 2021 · 4 comments · May be fixed by #903

Comments

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Nov 26, 2021

Problem description

The listdir() and rmdir() signatures are inconsistent across this class hierarchy:

BaseStore                             zarr/_storage/store.py
└── Store                             zarr/_storage/store.py
    ├── ABSStore                      zarr/_storage/absstore.py
    ├── MemoryStore                   zarr/storage.py
    ├── DirectoryStore                zarr/storage.py
    ├── FSStore                       zarr/storage.py
    ├── ZipStore                      zarr/storage.py
    ├── LRUStoreCache                 zarr/storage.py
    ├── SQLiteStore                   zarr/storage.py
    └── ConsolidatedMetadataStore     zarr/storage.py
class Store(BaseStore):
    [...]
    def listdir(self, path: str = "") -> List[str]:
    [...]
    def rmdir(self, path: str = "") -> None:
    [...]

class ABSStore(Store):
    [...]
    def listdir(self, path=None):
    [...]
    def rmdir(self, path=None):
    [...]

class MemoryStore(Store):
    [...]
    def listdir(self, path: Path = None) -> List[str]:
    [...]
    def rmdir(self, path: Path = None):
    [...]

class DirectoryStore(Store):
    [...]
    def listdir(self, path=None):
    [...]
    def rmdir(self, path=None):
    [...]

class FSStore(Store):
    [...]
    def listdir(self, path=None):
    [...]
    def rmdir(self, path=None):
    [...]

class ZipStore(Store):
    [...]
    def listdir(self, path=None):
    [...]

class LRUStoreCache(Store):
    [...]
    def listdir(self, path: Path = None):
    [...]

class SQLiteStore(Store):
    [...]
    def listdir(self, path=None):
    [...]
    def rmdir(self, path=None):
    [...]

class ConsolidatedMetadataStore(Store):
    [...]
    def listdir(self, path):
    [...]
  • Argument path should consistently be either a str or a Path
  • The default value of path should consistently be either "" or None.
  • ConsolidatedMetadataStore.listdir() needs a default value.
@DimitriPapadopoulos DimitriPapadopoulos linked a pull request Dec 2, 2021 that will close this issue
6 tasks
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Dec 2, 2021

One problem is that BaseStore/Store are defined in zarr/_storage/store.py, where Path has not been defined so far, while most subclasses are defined in zarr/storage.py, which defines:

Path = Union[str, bytes, None]

@DimitriPapadopoulos
Copy link
Contributor Author

But then why bytes? All paths should be str, shouldn't they?

@joshmoore
Copy link
Member

@jakirkham: did you ever get a chance to look at this one?

@DimitriPapadopoulos
Copy link
Contributor Author

Can Path be replaced by pathlib.Path? If not, re-using a well-known class name is confusing. Otherwise, switching should not be a problem for Python >= 3.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants