From 769785fba544f550f743549c673713b90b5d3513 Mon Sep 17 00:00:00 2001 From: David Stansby Date: Wed, 30 Jul 2025 12:46:40 +0100 Subject: [PATCH] Refactor make_store_path for clarity --- src/zarr/storage/_common.py | 99 ++++++++++++++++++--------------- tests/test_store/test_fsspec.py | 4 +- 2 files changed, 57 insertions(+), 46 deletions(-) diff --git a/src/zarr/storage/_common.py b/src/zarr/storage/_common.py index e25fa28424..3a63b30e9b 100644 --- a/src/zarr/storage/_common.py +++ b/src/zarr/storage/_common.py @@ -323,54 +323,65 @@ async def make_store_path( """ from zarr.storage._fsspec import FsspecStore # circular import - used_storage_options = False path_normalized = normalize_path(path) - if isinstance(store_like, StorePath): - result = store_like / path_normalized - else: - assert mode in (None, "r", "r+", "a", "w", "w-") - # if mode 'r' was provided, we'll open any new stores as read-only - _read_only = mode == "r" - if isinstance(store_like, Store): - store = store_like - elif store_like is None: - store = await MemoryStore.open(read_only=_read_only) - elif isinstance(store_like, Path): - store = await LocalStore.open(root=store_like, read_only=_read_only) - elif isinstance(store_like, str): - storage_options = storage_options or {} - - if _is_fsspec_uri(store_like): - used_storage_options = True - store = FsspecStore.from_url( - store_like, storage_options=storage_options, read_only=_read_only - ) - else: - store = await LocalStore.open(root=Path(store_like), read_only=_read_only) - elif isinstance(store_like, dict): - # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings. - # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate. - store = await MemoryStore.open(store_dict=store_like, read_only=_read_only) - elif _has_fsspec and isinstance(store_like, FSMap): - if path: - raise ValueError( - "'path' was provided but is not used for FSMap store_like objects. Specify the path when creating the FSMap instance instead." - ) - if storage_options: - raise ValueError( - "'storage_options was provided but is not used for FSMap store_like objects. Specify the storage options when creating the FSMap instance instead." - ) - store = FsspecStore.from_mapper(store_like, read_only=_read_only) - else: - raise TypeError(f"Unsupported type for store_like: '{type(store_like).__name__}'") - result = await StorePath.open(store, path=path_normalized, mode=mode) + if ( + not (isinstance(store_like, str) and _is_fsspec_uri(store_like)) + and storage_options is not None + ): + raise TypeError( + "'storage_options' was provided but unused. " + "'storage_options' is only used when the store is passed as a FSSpec URI string.", + ) - if storage_options and not used_storage_options: - msg = "'storage_options' was provided but unused. 'storage_options' is only used for fsspec filesystem stores." - raise TypeError(msg) + assert mode in (None, "r", "r+", "a", "w", "w-") + _read_only = mode == "r" - return result + if isinstance(store_like, StorePath): + # Already a StorePath + return store_like / path_normalized + + elif isinstance(store_like, Store): + # Already a Store + store = store_like + + elif isinstance(store_like, dict): + # Already a dictionary that can be a MemoryStore + # + # We deliberate only consider dict[str, Buffer] here, and not arbitrary mutable mappings. + # By only allowing dictionaries, which are in-memory, we know that MemoryStore appropriate. + store = await MemoryStore.open(store_dict=store_like, read_only=_read_only) + + elif store_like is None: + # Create a new in-memory store + return await make_store_path({}, path=path, mode=mode, storage_options=storage_options) + + elif isinstance(store_like, Path): + # Create a new LocalStore + store = await LocalStore.open(root=store_like, read_only=_read_only) + + elif isinstance(store_like, str): + # Either a FSSpec URI or a local filesystem path + if _is_fsspec_uri(store_like): + store = FsspecStore.from_url( + store_like, storage_options=storage_options, read_only=_read_only + ) + else: + # Assume a filesystem path + return await make_store_path( + Path(store_like), path=path, mode=mode, storage_options=storage_options + ) + + elif _has_fsspec and isinstance(store_like, FSMap): + if path: + raise ValueError( + "'path' was provided but is not used for FSMap store_like objects. Specify the path when creating the FSMap instance instead." + ) + store = FsspecStore.from_mapper(store_like, read_only=_read_only) + else: + raise TypeError(f"Unsupported type for store_like: '{type(store_like).__name__}'") + + return await StorePath.open(store, path=path_normalized, mode=mode) def _is_fsspec_uri(uri: str) -> bool: diff --git a/tests/test_store/test_fsspec.py b/tests/test_store/test_fsspec.py index 026b25f8fc..abee298a8c 100644 --- a/tests/test_store/test_fsspec.py +++ b/tests/test_store/test_fsspec.py @@ -388,8 +388,8 @@ def test_open_s3map_raises() -> None: ): zarr.open(store=mapper, path="bar", mode="w", shape=(3, 3)) with pytest.raises( - ValueError, - match="'storage_options was provided but is not used for FSMap store_like objects", + TypeError, + match="'storage_options' is only used when the store is passed as a FSSpec URI string.", ): zarr.open(store=mapper, storage_options={"anon": True}, mode="w", shape=(3, 3))