Skip to content
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

add enter and exit methods to groups. #2691

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dikwickley
Copy link

@dikwickley dikwickley commented Jan 12, 2025

Fixes: #2619

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • Test coverage is 100% (Codecov passes)
  • GitHub Actions have all passed

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Will you also add this for arrays?

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! Let's also get some unit tests for this. It will be important that we exercise this across all stores and establish expectations about behavior after the store has been closed.

def __enter__(self) -> Self:
return self

def __exit__(self) -> None: # noqa: PYI036
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def __exit__(self) -> None: # noqa: PYI036
def __exit__(
self,
typ: type[BaseException] | None,
exc: BaseException | None,
tb: TracebackType | None
) -> None:

See https://docs.astral.sh/ruff/rules/bad-exit-annotation/

Copy link
Author

Choose a reason for hiding this comment

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

updated in d8f7d68. Also added tests.

@dikwickley dikwickley marked this pull request as ready for review January 13, 2025 20:55
@dikwickley dikwickley requested a review from jhamman January 13, 2025 20:56
@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 14, 2025
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

Super close. A few suggestions on how to make the tests just a bit better.


# attempt to open a group that does not exist.
with pytest.raises(FileNotFoundError):
with Group.open(store) as store:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with Group.open(store) as store:
with Group.open(store) as g:

assert group.store_path == spath

# Check if store was closed after exit.
assert not store._is_open
Copy link
Member

Choose a reason for hiding this comment

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

rather than using this private store api, let's try to just to use the group again and make sure it fails.

Suggested change
assert not store._is_open
with pytest.raises(ValueError):
group.attrs['path'] = 'bar'


# attempt to open a group that does not exist.
with pytest.raises(FileNotFoundError):
with Group.open(store) as store:
Copy link
Member

Choose a reason for hiding this comment

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

also, instead of using Group.open and Group.from_store, can we use zarr.open_group and zarr.create_group? These are the public facing API methods we want most folks to be using.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v3] Groups and Arrays do not implement __enter__ / __exit__ protocols
4 participants