Skip to content

Add a GroupNotFoundError #3066

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 29, 2025
Merged

Add a GroupNotFoundError #3066

merged 5 commits into from
May 29, 2025

Conversation

dstansby
Copy link
Contributor

Fixes #2298.

Note I also updated the base zarr exception to no longer inherit from ValueError, because in general errors we raise are not always ValueErrors. I'll add a separate release note for this change.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label May 16, 2025
@dstansby dstansby force-pushed the groupnotfounderror branch from 5d8f611 to fedf099 Compare May 21, 2025 11:40
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label May 21, 2025
@dstansby dstansby marked this pull request as ready for review May 21, 2025 12:18
@d-v-b
Copy link
Contributor

d-v-b commented May 21, 2025

would you want to get the other missing errors in here? you could take some of #3012

@d-v-b
Copy link
Contributor

d-v-b commented May 21, 2025

also worth noting that changing the type of the base exception is breaking, because try... catch ValueError would have worked before and will not after this change.

@dstansby
Copy link
Contributor Author

I didn't spot #3012, thanks! I'll put this back in draft and have a think when I've got a bit more time.

@dstansby dstansby marked this pull request as draft May 21, 2025 12:49
@dstansby dstansby marked this pull request as ready for review May 28, 2025 09:21
@dstansby
Copy link
Contributor Author

I reverted this to just adding a GroupNotFoundError, which isn't breaking and a self-contained change. This could be merged as is, but I'm +/- 0 on whether to try and put in more errors in the same PR. Pros: less confusing to users if we put all the new exceptions in in one go. Cons: harder to do/review. Anyone have any thoughts either way?

@d-v-b
Copy link
Contributor

d-v-b commented May 28, 2025

I reverted this to just adding a GroupNotFoundError, which isn't breaking and a self-contained change. This could be merged as is, but I'm +/- 0 on whether to try and put in more errors in the same PR. Pros: less confusing to users if we put all the new exceptions in in one go. Cons: harder to do/review. Anyone have any thoughts either way?

incremental progress is progress IMO, so I'm fine with just the one exception added here.

@dstansby dstansby requested a review from d-v-b May 29, 2025 08:33
d-v-b
d-v-b reviewed May 29, 2025
View reviewed changes
tests/test_metadata/test_v3.py
@@ -88,7 +89,8 @@ def test_parse_node_type_invalid(node_type: Any) -> None:
@pytest.mark.parametrize("data", [None, "group"])
def test_parse_node_type_array_invalid(data: Any) -> None:
with pytest.raises(
ValueError, match=f"Invalid value for 'node_type'. Expected 'array'. Got '{data}'."
NodeTypeValidationError,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering now why we have a special exception for this case...

@dstansby dstansby enabled auto-merge (squash) May 29, 2025 08:53
@dstansby dstansby merged commit af55fcf into zarr-developers:main May 29, 2025
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-v-b d-v-b d-v-b approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Raise specific errors when failing to open a group.
2 participants
@dstansby @d-v-b