Refactor: store mode and initialization #2442
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
| assert mode in ("r", "w") | ||
| self._mode = mode |
There was a problem hiding this comment.
we may consider a different setting here (e.g. readonly)
|
|
||
| @abstractmethod | ||
| def with_mode(self, mode: AccessModeLiteral) -> Self: | ||
| def with_mode(self, mode: StoreAccessMode) -> Self: |
There was a problem hiding this comment.
To limit the scope here, I decided to keep this. But I'm not convinced we need it anymore.
| else: | ||
| mode = "r+" | ||
| store_mode = _handle_store_mode(mode) | ||
| print(f"store_mode: {store_mode}") |
There was a problem hiding this comment.
| print(f"store_mode: {store_mode}") |
| def _handle_store_mode(mode: AccessModeLiteral | None) -> StoreAccessMode: | ||
| if mode is None: | ||
| return "r" | ||
| else: | ||
| return persistence_to_store_modes[mode] |
There was a problem hiding this comment.
What do we have to change higher up in the codebase if we narrow the function signature to mode: AccessModeLiteral? My gut feeling is that we should not need to handle None here, and the None -> r normalization can happen earlier.
| except (KeyError, FileNotFoundError): | ||
| pass | ||
| if mode in {"a", "w", "w-"}: |
There was a problem hiding this comment.
should we bind this set to a variable, and similarly for {"r", "r+", "a"}? that would improve the literacy a bit.
| match mode: | ||
| case "w-": | ||
| if not await self.empty(): | ||
| raise FileExistsError(f"{self} must be empty. Mode is 'w-'") |
There was a problem hiding this comment.
| raise FileExistsError(f"{self} must be empty. Mode is 'w-'") | |
| msg = ( | |
| f"{self} is not empty, but `mode` is set to 'w-'." | |
| "Either remove the existing objects in storage," | |
| "or set `mode` to a value that handles pre-existing objects" | |
| "in storage, like `a` or `w`." | |
| ) | |
| raise FileExistsError(msg) |
| FileExistsError | ||
| If the mode is 'w-' and the store path already exists. | ||
| """ | ||
| match mode: |
There was a problem hiding this comment.
is this our first match statement in the codebase?
| # try: | ||
| # del root[array_path] | ||
| # except KeyError: | ||
| # pass |
There was a problem hiding this comment.
| # try: | |
| # del root[array_path] | |
| # except KeyError: | |
| # pass |
| self._is_open = False | ||
| self._mode = AccessMode.from_literal(mode) | ||
| assert mode in ("r", "w") |
| """ | ||
| Check if the store is empty. | ||
| Check if the directory is empty. |
There was a problem hiding this comment.
are we going with object storage vocab or file system vocab? If the former, then maybe we should say something like "check if the store contains any keys with the given prefix"
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:Store.modebut allows this to be onlyStoreAccessMode = Literal["r", "w"].wmode.StorePath._init(mode: AccessModeLiteral)where side effects can be applied at the path levelbuilds on #2430
closes #2359
TODO: