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

document storage classes and some developer apis #2279

Merged
merged 37 commits into from
Oct 13, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Oct 1, 2024

closes #2250

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)

@jhamman jhamman added documentation Improvements to the documentation V3 Affects the v3 branch labels Oct 1, 2024
@jhamman jhamman linked an issue Oct 1, 2024 that may be closed by this pull request
2 tasks
@jhamman jhamman marked this pull request as ready for review October 2, 2024 04:47
@jhamman jhamman changed the base branch from fix/dask-compat to v3 October 2, 2024 04:47
@dstansby
Copy link
Contributor

dstansby commented Oct 2, 2024

Would be good to get this up to date with the v3 branch before reviewing

@jhamman
Copy link
Member Author

jhamman commented Oct 2, 2024

I can rebase this today.

@jhamman
Copy link
Member Author

jhamman commented Oct 7, 2024

Okay, @dstansby (and others), this is now ready for a review.

Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I've left some comments - one big one, is why so many docstrings deleted? Is it because they're inherited? If so, would be good to include a # docstring inherited comment where they are inherited so others don't get confused in the future.

Parameters
----------
mode : AccessModeLiteral
One of 'r', 'r+', 'w', 'w-', 'a'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we define these as a string somewhere, and then re-use it here and lower down by making the docstring a format string? I worry about lists like this getting out of sync if they're duplicated across docstrings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided not to do that here because its just this one method. If we have a docstring template tool in the future, I think it would be great to bring that to bear here (and even more so on the Group/Array classes).

src/zarr/abc/store.py Outdated Show resolved Hide resolved
src/zarr/abc/store.py Outdated Show resolved Hide resolved
src/zarr/abc/store.py Outdated Show resolved Hide resolved
src/zarr/storage/common.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
src/zarr/storage/remote.py Outdated Show resolved Hide resolved
@jhamman
Copy link
Member Author

jhamman commented Oct 10, 2024

@dstansby - ready for another review here.

@jhamman jhamman requested a review from dstansby October 10, 2024 23:48
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I think you can get rid of the custom docstring inheritance code (see my inline comment). Where a docstring is inherited though, please could you leave a # docstring inherited comment under the signature, to signal to others that come by later that a fresh docstring doesn't need writing.

@@ -151,3 +151,33 @@ def parse_order(data: Any) -> Literal["C", "F"]:
if data in ("C", "F"):
return cast(Literal["C", "F"], data)
raise ValueError(f"Expected one of ('C', 'F'), got {data} instead.")


def _inherit_docstrings(cls: type[Any]) -> type[Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstrings are already automatically inherited, e.g. see Store and LocalStore. So I think this isn't needed and can be gotten rid of.

@jhamman jhamman requested a review from dstansby October 11, 2024 21:29
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Good for me - might be worth getting someone more familiar with v3 to look over the docstring contents?

@dstansby
Copy link
Contributor

:shipit:

@dstansby dstansby merged commit d2dc162 into zarr-developers:v3 Oct 13, 2024
20 checks passed
@jhamman jhamman added this to the 3.0.0 milestone Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements to the documentation V3 Affects the v3 branch
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add documentation for zarr.storage
2 participants