-
-
Notifications
You must be signed in to change notification settings - Fork 329
Normalize paths when creating StorePath #2850
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
Changes from all commits
c0feec5
322acf7
9930e30
f342bb9
de84391
64ef489
392e4f7
c5ad6fb
38e5854
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fixed a bug where ``StorePath`` creation would not apply standard path normalization to the ``path`` parameter, | ||
which led to the creation of arrays and groups with invalid keys. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,24 +130,27 @@ async def test_create_creates_parents(store: Store, zarr_format: ZarrFormat) -> | |
assert g.attrs == {} | ||
|
||
|
||
def test_group_name_properties(store: Store, zarr_format: ZarrFormat) -> None: | ||
@pytest.mark.parametrize("store", ["memory"], indirect=True) | ||
@pytest.mark.parametrize("root_name", ["", "/", "a", "/a"]) | ||
@pytest.mark.parametrize("branch_name", ["foo", "/foo", "foo/bar", "/foo/bar"]) | ||
def test_group_name_properties( | ||
store: Store, zarr_format: ZarrFormat, root_name: str, branch_name: str | ||
) -> None: | ||
""" | ||
Test basic properties of groups | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think the old test here is useful (in addition to the updated test), because it shows explicitly what the values of all the properties are, making it much easier to see what's changed if they change (which they might have done with this PR). So I'd be pro just copy/pasting the old test back here along with the new test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the new test is the same as the old test, but parametrized instead of 2 cases copy + pasted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as a rule we should avoid tests that follow the "copy + pasted code blocks with minor modifications" pattern. these are huge source of friction when fixing bugs. that friction should be reduced by using parametrization where we can, as I did here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree that we shouldn't copy/paste with minor modifications, but I think one test that hard codes the results instead of using other properties is worthwhile to have. |
||
Test that the path, name, and basename attributes of a group and its subgroups are consistent | ||
""" | ||
root = Group.from_store(store=store, zarr_format=zarr_format) | ||
assert root.path == "" | ||
assert root.name == "/" | ||
assert root.basename == "" | ||
root = Group.from_store(store=StorePath(store=store, path=root_name), zarr_format=zarr_format) | ||
assert root.path == normalize_path(root_name) | ||
assert root.name == "/" + root.path | ||
assert root.basename == root.path | ||
|
||
foo = root.create_group("foo") | ||
assert foo.path == "foo" | ||
assert foo.name == "/foo" | ||
assert foo.basename == "foo" | ||
|
||
bar = root.create_group("foo/bar") | ||
assert bar.path == "foo/bar" | ||
assert bar.name == "/foo/bar" | ||
assert bar.basename == "bar" | ||
branch = root.create_group(branch_name) | ||
if root.path == "": | ||
assert branch.path == normalize_path(branch_name) | ||
else: | ||
assert branch.path == "/".join([root.path, normalize_path(branch_name)]) | ||
assert branch.name == "/" + branch.path | ||
assert branch.basename == branch_name.split("/")[-1] | ||
|
||
|
||
@pytest.mark.parametrize("consolidated_metadata", [True, False]) | ||
|
@@ -623,11 +626,13 @@ async def test_group_update_attributes_async(store: Store, zarr_format: ZarrForm | |
|
||
|
||
@pytest.mark.parametrize("method", ["create_array", "array"]) | ||
@pytest.mark.parametrize("name", ["a", "/a"]) | ||
def test_group_create_array( | ||
store: Store, | ||
zarr_format: ZarrFormat, | ||
overwrite: bool, | ||
method: Literal["create_array", "array"], | ||
name: str, | ||
) -> None: | ||
""" | ||
Test `Group.from_store` | ||
|
@@ -638,23 +643,26 @@ def test_group_create_array( | |
data = np.arange(np.prod(shape)).reshape(shape).astype(dtype) | ||
|
||
if method == "create_array": | ||
array = group.create_array(name="array", shape=shape, dtype=dtype) | ||
array = group.create_array(name=name, shape=shape, dtype=dtype) | ||
array[:] = data | ||
elif method == "array": | ||
with pytest.warns(DeprecationWarning): | ||
array = group.array(name="array", data=data, shape=shape, dtype=dtype) | ||
array = group.array(name=name, data=data, shape=shape, dtype=dtype) | ||
else: | ||
raise AssertionError | ||
|
||
if not overwrite: | ||
if method == "create_array": | ||
with pytest.raises(ContainsArrayError): | ||
a = group.create_array(name="array", shape=shape, dtype=dtype) | ||
a = group.create_array(name=name, shape=shape, dtype=dtype) | ||
a[:] = data | ||
elif method == "array": | ||
with pytest.raises(ContainsArrayError), pytest.warns(DeprecationWarning): | ||
a = group.array(name="array", shape=shape, dtype=dtype) | ||
a = group.array(name=name, shape=shape, dtype=dtype) | ||
a[:] = data | ||
|
||
assert array.path == normalize_path(name) | ||
dcherian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert array.name == "/" + array.path | ||
assert array.shape == shape | ||
assert array.dtype == np.dtype(dtype) | ||
assert np.array_equal(array[:], data) | ||
|
@@ -945,20 +953,23 @@ async def test_asyncgroup_delitem(store: Store, zarr_format: ZarrFormat) -> None | |
raise AssertionError | ||
|
||
|
||
@pytest.mark.parametrize("name", ["a", "/a"]) | ||
async def test_asyncgroup_create_group( | ||
store: Store, | ||
name: str, | ||
zarr_format: ZarrFormat, | ||
) -> None: | ||
agroup = await AsyncGroup.from_store(store=store, zarr_format=zarr_format) | ||
sub_node_path = "sub_group" | ||
attributes = {"foo": 999} | ||
subnode = await agroup.create_group(name=sub_node_path, attributes=attributes) | ||
|
||
assert isinstance(subnode, AsyncGroup) | ||
assert subnode.attrs == attributes | ||
assert subnode.store_path.path == sub_node_path | ||
assert subnode.store_path.store == store | ||
assert subnode.metadata.zarr_format == zarr_format | ||
subgroup = await agroup.create_group(name=name, attributes=attributes) | ||
|
||
assert isinstance(subgroup, AsyncGroup) | ||
assert subgroup.path == normalize_path(name) | ||
assert subgroup.name == "/" + subgroup.path | ||
assert subgroup.attrs == attributes | ||
assert subgroup.store_path.path == subgroup.path | ||
assert subgroup.store_path.store == store | ||
assert subgroup.metadata.zarr_format == zarr_format | ||
|
||
|
||
async def test_asyncgroup_create_array( | ||
|
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.
shall we delete line 287 then.
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.
64ef489