-
-
Notifications
You must be signed in to change notification settings - Fork 365
Refactor: store mode and initialization #2442
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
Conversation
overwriting nodes - add Store.delete_dir and Store.delete_prefix - update array and group creation methods to call delete_dir - change list_prefix to return absolue keys
…into feature/store_erase_prefix
…python into feature/store_erase_prefix
…into feature/store_erase_prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes to guide review
src/zarr/abc/store.py
Outdated
| assert mode in ("r", "w") | ||
| self._mode = mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may consider a different setting here (e.g. readonly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally prefer the attribute matching the parameter name.
We can have a property
@property
def readonly(self): return self.mode == "r"
Edit: which I see you have down below.
Co-authored-by: Davis Bennett <[email protected]>
…into refactor/store-mode
…into refactor/store-mode
|
Thanks folks for the reviews here! |
Previously, opening a store in
wmode, had a side effect (Store.clear). That was leading to unexpected behavior when apathargument was also supplied. This refactor takes the following approach:keepsStore.modebut allows this to be onlyStoreAccessMode = Literal["r", "w"]Store.modewithStore.read_onlyStorePath.open(..., mode: AccessModeLiteral)where side effects can be applied at the path levelbuilds on #2430
closes #2359
TODO: