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

Implement LindiReferenceFileSystemStore.__contains__ #75

Merged
merged 2 commits into from
May 28, 2024
Merged

Conversation

rly
Copy link
Contributor

@rly rly commented May 27, 2024

MutableMapping implements __contains__ by calling __getitem__ and checking for None. Zarr BaseStore has this line:
https://github.com/zarr-developers/zarr-python/blob/b1f4c509abaee1cb8dec18e3a973e1199226011a/src/zarr/v2/_storage/store.py#L142

which does a both __contains__ check and __getitem__ check. That results in two remote requests per __getitem__ call in LindiReferenceFileSystemStore.

This PR removes the remote request for __contains__.

@rly rly requested a review from magland May 27, 2024 23:43
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.43%. Comparing base (5922b4f) to head (4fdab5f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #75      +/-   ##
==========================================
+ Coverage   79.41%   79.43%   +0.01%     
==========================================
  Files          30       30              
  Lines        2254     2256       +2     
==========================================
+ Hits         1790     1792       +2     
  Misses        464      464              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@magland
Copy link
Collaborator

magland commented May 28, 2024

Nice catch!

@magland magland merged commit ac9b6f7 into main May 28, 2024
6 checks passed
@magland magland deleted the add_contains branch May 28, 2024 13:53
@rly
Copy link
Contributor Author

rly commented May 28, 2024

FYI I released this on PyPI as 0.3.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants