Skip to content

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Jul 29, 2025

Fixes #3295. I'm not sure if I came up with the most elegant solution, but I couldn't think of a way to solve this without passing the mode to LocalStore.open(). Other suggestions very welcome if anyone has any better ideas?

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 29, 2025
@dstansby dstansby force-pushed the r+ branch 2 times, most recently from d84eada to ee21ccf Compare July 30, 2025 12:13
@dstansby dstansby added this to the 3.1.2 milestone Jul 30, 2025
@dstansby dstansby force-pushed the r+ branch 2 times, most recently from 8ddf8d4 to 9342439 Compare August 4, 2025 11:04
@dstansby dstansby marked this pull request as ready for review August 4, 2025 12:29
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.70%. Comparing base (dc641fe) to head (288ce96).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3307   +/-   ##
=======================================
  Coverage   94.69%   94.70%           
=======================================
  Files          79       79           
  Lines        9520     9532   +12     
=======================================
+ Hits         9015     9027   +12     
  Misses        505      505           
Files with missing lines Coverage Δ
src/zarr/storage/_common.py 92.43% <100.00%> (ø)
src/zarr/storage/_local.py 92.21% <100.00%> (+0.60%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Aug 20, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Aug 25, 2025

this works but I find the logic hard to follow. I think that's a side effect of our failure to distinguish two separate things:

  1. is the store instance allowed to modify existing objects
  2. is the store instance allowed to create new objects

these are technically two separate permissions, but for zarr purposes, 2 implies 1.

We need to formalize these differences higher up in the store permissions model I think.

@d-v-b d-v-b merged commit 0e28404 into zarr-developers:main Aug 25, 2025
31 checks passed
Copy link

lumberbot-app bot commented Aug 25, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 3.1.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 0e28404b9952008a1604e561afd71725f04eeb07
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am "Backport PR #3307: Prevent mode='r+' from creating new directories"
  1. Push to a named branch:
git push YOURFORK 3.1.x:auto-backport-of-pr-3307-on-3.1.x
  1. Create a PR against branch 3.1.x, I would have named this PR:

"Backport PR #3307 on branch 3.1.x (Prevent mode='r+' from creating new directories)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using r+ mode with zarr.open creates an empty directory

2 participants