fix/normalize storage paths #2384
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhamman
reviewed
Oct 15, 2024
jhamman
reviewed
Oct 15, 2024
Co-authored-by: Joe Hamman <[email protected]>
…x/normalize-storage-paths
There was a problem hiding this comment.
Thanks, a couple questions about the implementation.
One meta-comment I've been thinking about, probably for a future PR: we could define PathLike as a NewType. The user-facing API could accept str | PathLike, and internally we could use just PathLike. Then we can verify with mypy that we've normalized all paths provided by the user.
TomAugspurger
approved these changes
Oct 16, 2024
d-v-b
added a commit
to d-v-b/zarr-python
that referenced
this pull request
Oct 18, 2024
* bring in path normalization function from v2, and add a failing test * rephrase comment * simplify storepath creation * Update tests/v3/test_api.py Co-authored-by: Joe Hamman <[email protected]> * refactor: remove redundant zarr format fixture * replace assertion with an informative error message * fix incorrect path concatenation in make_store_path, and refactor store_path tests * remove upath import because we don't need it * apply suggestions from code review --------- Co-authored-by: Joe Hamman <[email protected]>
d-v-b
added a commit
that referenced
this pull request
Oct 18, 2024
* move v3/tests to tests and fix various mypy issues * test(ci): change branch name in v3 workflows (#2368) * Use lazy % formatting in logging functions (#2366) * Use lazy % formatting in logging functions * f-string should be more efficient * Space before unit symbol From "SI Unit rules and style conventions": https://physics.nist.gov/cuu/Units/checklist.html There is a space between the numerical value and unit symbol, even when the value is used in an adjectival sense, except in the case of superscript units for plane angle. * Enforce ruff/flake8-logging-format rules (G) --------- Co-authored-by: Joe Hamman <[email protected]> * Move roadmap and v3-design documument to docs (#2354) * move roadmap to docs * formatting and minor copy editing * Multiple imports for an import name (#2367) Co-authored-by: Joe Hamman <[email protected]> * Enforce ruff/pycodestyle warnings (W) (#2369) * Apply ruff/pycodestyle rule W291 W291 Trailing whitespace * Enforce ruff/pycodestyle warnings (W) It looks like `ruff format` does not catch all trailing spaces. --------- Co-authored-by: Joe Hamman <[email protected]> * Apply ruff/pycodestyle preview rule E262 (#2370) E262 Inline comment should start with `# ` Co-authored-by: Joe Hamman <[email protected]> * Fix typo (#2382) Co-authored-by: Joe Hamman <[email protected]> * Imported name is not used anywhere in the module (#2379) * Missing mandatory keyword argument `shape` (#2376) * Update ruff rules to ignore (#2374) Co-authored-by: Joe Hamman <[email protected]> * Docstrings for arraymodule (#2276) * start to docstrings for arraymodule * incorporating toms edits, overriding mypy error... * fix attrs * Update src/zarr/core/array.py Co-authored-by: Sanket Verma <[email protected]> * fix store -> storage * remove properties from asyncarray docstring --------- Co-authored-by: Sanket Verma <[email protected]> Co-authored-by: Joe Hamman <[email protected]> * fix/normalize storage paths (#2384) * bring in path normalization function from v2, and add a failing test * rephrase comment * simplify storepath creation * Update tests/v3/test_api.py Co-authored-by: Joe Hamman <[email protected]> * refactor: remove redundant zarr format fixture * replace assertion with an informative error message * fix incorrect path concatenation in make_store_path, and refactor store_path tests * remove upath import because we don't need it * apply suggestions from code review --------- Co-authored-by: Joe Hamman <[email protected]> * Enforce ruff/flake8-pyi rule PYI013 (#2389) PYI013 Non-empty class body must not contain `...` Note that documentation is enough to fill the class body. * deps: remove fasteners from list of dependencies (#2386) * Enforce ruff/flake8-annotations rule ANN003 (#2388) ANN003 Missing type annotation Co-authored-by: Joe Hamman <[email protected]> * Enforce ruff/Perflint rules (PERF) (#2372) * Apply ruff/Perflint rule PERF401 PERF401 Use a list comprehension to create a transformed list * Enforce ruff/Perflint rules (PERF) * chore: update package maintainers (#2387) * chore: update package maintainers * Update pyproject.toml Co-authored-by: David Stansby <[email protected]> --------- Co-authored-by: David Stansby <[email protected]> * Fixed consolidated Group getitem with multi-part key (#2363) * Fixed consolidated Group getitem with multi-part key This fixes `Group.__getitem__` when indexing with a key like 'subgroup/array'. The basic idea is to rewrite the indexing operation as `group['subgroup']['array']` by splitting the key and doing each operation independently. Closes #2358 --------- Co-authored-by: Joe Hamman <[email protected]> * chore: add python 3.13 to ci / pyproject.toml (#2385) * chore: add python 3.13 to ci / pyproject.toml * update hatch matrix * remove references to dead test dir in pyproject.toml * remove v3 reference in test --------- Co-authored-by: Joe Hamman <[email protected]> Co-authored-by: Dimitri Papadopoulos Orfanos <[email protected]> Co-authored-by: Emma Marshall <[email protected]> Co-authored-by: Sanket Verma <[email protected]> Co-authored-by: David Stansby <[email protected]> Co-authored-by: Tom Augspurger <[email protected]>
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds some of the path normalization logic from zarr v2 into v3. I largely copied this function as-is with a few tweaks. That function is actually not terribly strict as far as path parsing is concerned, so I'm open to requests that we make it stricter. The main thing to note is that
normalize_path(the new name for that function) strips leading/characters from strings, which ensures that paths are relative.I wired this function up to
make_store_path, which previously didn't takepathas a parameter (now it does). This led to some nice code deletion opportunities in our various creation routines.Something that always bugged me in zarr v2 was use of
Noneas a default value for types where there's already a default value. In the case of strings, the empty string''is a perfectly good default value, and so allowingNoneis just noise IMO. I was tempted to try and removeNoneas a valid path argument in this PR, but I held back. If people agree thatNoneis a silly default for a stringy value likepathwhen we can just use''instead, then I can make that change as well.Fixes #2357
TODO: