Skip to content

Rename StorePath to PathStore? #2905

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
dstansby opened this issue Mar 11, 2025 · 8 comments · Fixed by #2976
Closed

Rename StorePath to PathStore? #2905

dstansby opened this issue Mar 11, 2025 · 8 comments · Fixed by #2976
Labels
documentation Improvements to the documentation

Comments

@dstansby
Copy link
Contributor

All the other stores are named *Store, but StorePath breaks this convention. It might be nice to add a PathStore class, and then eventually deprecate and remove StorePath?

@d-v-b
Copy link
Contributor

d-v-b commented Mar 11, 2025

StorePath is not a store, it wraps a store and a path.

@d-v-b
Copy link
Contributor

d-v-b commented Mar 11, 2025

but I agree more broadly that StorePath is a confusing entity. My preferred change would be to implement all of its behavior on store instances themselves, and then have no need for a separate StorePath class.

@dstansby
Copy link
Contributor Author

Ah, so is it really an abstract base class (or intended to be?)

@d-v-b
Copy link
Contributor

d-v-b commented Mar 11, 2025

no, StorePath is a regular class that gets instantiated, and it has methods that get used. It compensates for the fact that our Store API only formally supports absolute paths, but for a lot of Zarr IO, paths relative to some other path (e.g., the path to a group) are more convenient.

@jhamman
Copy link
Member

jhamman commented Apr 10, 2025

@dstansby - okay if we close this as "not planned"?

@dstansby
Copy link
Contributor Author

I think we should leave it open, but instead of changing the name split up the API doc at https://zarr.readthedocs.io/en/stable/api/zarr/storage/index.html#classes so all the stores are listed together, and StorePath (and antyhgin else that isn't a store) is listed separately.

@dstansby dstansby added the documentation Improvements to the documentation label Apr 10, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Apr 10, 2025

I don't think we need to add complexity to the API docs here. It's simplest if the docs page for the storage module documents all the public stuff in that module. If people think StorePath is a store, they can very quickly find out that it is not, by reading the documentation for that class.

@dstansby
Copy link
Contributor Author

yeah, that's fair - I've suggested an alternative at #2976 to make the docstrings consistent, which I think would improve things.

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

Successfully merging a pull request may close this issue.

3 participants