Skip to content

Pathlib support #768

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

Merged
merged 8 commits into from
Aug 17, 2021
Merged

Pathlib support #768

merged 8 commits into from
Aug 17, 2021

Conversation

clbarnes
Copy link
Contributor

@clbarnes clbarnes commented Jun 4, 2021

Minor changes to utility functions to allow convenience functions to work with os.PathLikes; see #261 and #601. These now just normalise to str, which the DirectoryStore was doing explicitly before anyway. It might be nice to avoid that and for the path member of things containing a path to store (/return) it as a Path, but that was a more invasive change; the main value here is avoiding the errors.

One module defined a Path newtype for typing purposes; I renamed that to PathType for clarity.

I found it a little confusing to see "path" used to describe zarr's internal keys; looking through the code, you have to get whether it's talking about key or e.g. file system path from context. Would changing that verbiage be considered in an upcoming major version increment? Another "going forward" thought: it's a bit of a pain/ source of errors to manipulate path-like things as strings, which is why pathlib exists. Would you consider something like

from pathlib import PurePosixPath

class ZarrKey(PurePosixPath):
    pass

for internal use to begin with? On the flip side, we'd need to be more careful about os.PathLike checks, but there are only a couple in there.

Lastly, where in release.rst do new features go? There doesn't seem to be an entry for "current release" features.

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
    • Nothing to document as I would assume this behaviour from anything interacting with the file system
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #768 (24ca450) into master (2241a26) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #768   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files          29       30    +1     
  Lines       10568    10586   +18     
=======================================
+ Hits        10562    10580   +18     
  Misses          6        6           
Impacted Files Coverage Δ
zarr/convenience.py 100.00% <100.00%> (ø)
zarr/creation.py 100.00% <100.00%> (ø)
zarr/tests/conftest.py 100.00% <100.00%> (ø)
zarr/tests/test_convenience.py 100.00% <100.00%> (ø)
zarr/tests/test_storage.py 100.00% <100.00%> (ø)

@clbarnes clbarnes changed the title Pathlib Pathlib support Jun 4, 2021
@joshmoore
Copy link
Member

Would you consider something like ... class ZarrKey(PurePosixPath) for internal use to begin with?

Interesting. I'd be especially happy if I could attach extra metadata to that ;) see #769 (comment)

Lastly, where in release.rst do new features go?

Just add to the top. (The mainline may now have an "Unreleased" section at the top)

@clbarnes
Copy link
Contributor Author

clbarnes commented Jun 24, 2021

Rebased and added documentation to release.rst. Build failures are (I hope) unrelated; a segfault coming from numpy.

@clbarnes
Copy link
Contributor Author

clbarnes commented Jul 2, 2021

Rebased and green!

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.

In general, looks good from my side. I've noted one renaming issue on which I'd appreciate thoughts from others.

@clbarnes
Copy link
Contributor Author

Now rebased on master; I've also reverted the Path rename and have used import pathlib; ... pathlib.Path everywhere it's used to be maximally explicit.

This makes a more obvious contrast opposed to zarr's built-in `Path`
type annotation (for internal keys).
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.

Thanks for the rebase, @clbarnes! To my eye, this is now a very non-invasive widening of what the open methods will accept.

@joshmoore joshmoore merged commit ce04aaa into zarr-developers:master Aug 17, 2021
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 this pull request may close these issues.

2 participants