Skip to content

Refactor make_store_path for clarity #3308

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 4, 2025
Merged

Refactor make_store_path for clarity #3308

merged 1 commit into from
Aug 4, 2025

Conversation

dstansby
Copy link
Contributor

This refactors make_store_path to make the code easier to read by:

  • Moving the TypeError for unused storage_options to the top of the funciton
  • This then allows the if/else logic to be made one level deep, where each branch is for a different store type. Previously there were two levels of if/else.

There should not be any behaviour changes here.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 30, 2025
@dstansby dstansby mentioned this pull request Jul 30, 2025
Open
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Jul 30, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Jul 30, 2025

nice! the ultimate refactor would be removing the StorePath class entirely, and adding a path attribute to the store class, and giving the store classes all of the StorePath methods. But what you have here is a much easier lift

@dstansby dstansby force-pushed the refactor-make-store-path branch from 6ecd146 to 2da031f Compare July 30, 2025 11:20
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 30, 2025
@dstansby dstansby force-pushed the refactor-make-store-path branch 3 times, most recently from c16beef to 92f4e89 Compare July 30, 2025 11:46
@codecov Codecov
Copy link

codecov bot commented Jul 30, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.69%. Comparing base (e410173) to head (769785f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3308      +/-   ##
==========================================
- Coverage   60.72%   60.69%   -0.03%     
==========================================
  Files          78       78              
  Lines        9408     9401       -7     
==========================================
- Hits         5713     5706       -7     
  Misses       3695     3695
Files with missing lines Coverage Δ
src/zarr/storage/_common.py 68.10% <100.00%> (-1.17%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dstansby dstansby added this to the 3.1.2 milestone Jul 30, 2025
@dstansby dstansby requested a review from a team August 1, 2025 13:40
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 1, 2025
@dstansby dstansby force-pushed the refactor-make-store-path branch from 92f4e89 to 769785f Compare August 1, 2025 21:49
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Aug 1, 2025
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 4, 2025
@dstansby dstansby requested a review from d-v-b August 4, 2025 09:54
@dstansby dstansby mentioned this pull request Aug 4, 2025
Open
6 tasks
d-v-b
d-v-b approved these changes Aug 4, 2025
View reviewed changes
Copy link
Contributor

@d-v-b d-v-b left a comment

Choose a reason for hiding this comment

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

looks good. We should absolutely consider doing away with StorePath entirely, but that's a later PR

@dstansby dstansby merged commit 6547b7f into zarr-developers:main Aug 4, 2025
31 checks passed
@dstansby dstansby deleted the refactor-make-store-path branch August 4, 2025 10:12
meeseeksmachine pushed a commit to meeseeksmachine/zarr-python that referenced this pull request Aug 4, 2025
@dstansby @meeseeksmachine
Backport PR zarr-developers#3308: Refactor make_store_path for clarity
076e6b2
@meeseeksmachine meeseeksmachine mentioned this pull request Aug 4, 2025
Merged
dstansby added a commit to meeseeksmachine/zarr-python that referenced this pull request Aug 4, 2025
@dstansby
Backport PR zarr-developers#3308: Refactor make_store_path for clarity
b7efa1f
dstansby added a commit that referenced this pull request Aug 4, 2025
@meeseeksmachine @dstansby
Backport PR #3308: Refactor make_store_path for clarity (#3327)
31852f5 
Co-authored-by: David Stansby <dstansby@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-v-b d-v-b d-v-b approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.
2 participants
@dstansby @d-v-b