Conversation
The `SafeStore.__getitem__` method (formerly `ZarrNotFilledStore`) was missing a test for the `KeyError` exception path where it should raise a `FileNotFoundError`. This is important for ensuring that zarr propagates the exception rather than filling. This PR adds: - `tests/test_util_safestore.py`: A new unit test file for the `SafeStore` class. Coverage: - Happy path for `__getitem__`. - `KeyError` path for `__getitem__` (raising `FileNotFoundError`). - `__len__`, `__iter__`, `__getattr__` (passthrough and `__setstate__` restriction). - `__setitem__` and `__delitem__` (raising `NotImplementedError`). The improvement ensures better reliability and coverage for this critical data-access utility.
There was a problem hiding this comment.
Pull request overview
This PR’s stated goal is to add unit tests for SafeStore.__getitem__ error translation (KeyError → FileNotFoundError). However, the diff also includes a large number of new files under venv/Lib/site-packages/… (including pip and _distutils_hack), which looks like an accidentally committed virtual environment and does not align with the PR description.
Changes:
- Add
tests/test_util_safestore.pyto coverSafeStorebehaviors (includingKeyErrortranslation). - Add many files under
venv/Lib/site-packages/...(pip internals and dist-info), likely from a local virtualenv (unexpected / unrelated to SafeStore tests).
Reviewed changes
Copilot reviewed 107 out of 756 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| venv/Lib/site-packages/pip/_internal/req/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/pyproject.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/install/legacy.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/install/editable_legacy.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/install/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/freeze.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/check.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/wheel_legacy.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/wheel_editable.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/wheel.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/metadata_legacy.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/metadata_editable.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/metadata.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/operations/build/build_tracker.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/xmlrpc.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/utils.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/lazy_wheel.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/download.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/cache.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/network/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/wheel.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/target_python.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/selection_prefs.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/search_scope.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/scheme.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/installation_report.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/index.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/format_control.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/direct_url.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/candidate.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/models/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/importlib/_envs.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/importlib/_dists.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/importlib/_compat.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/importlib/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/_json.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/metadata/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/main.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/locations/base.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/locations/_sysconfig.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/locations/_distutils.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/index/sources.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/index/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/distributions/wheel.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/distributions/sdist.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/distributions/installed.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/distributions/base.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/distributions/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/wheel.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/uninstall.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/show.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/search.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/inspect.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/index.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/help.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/hash.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/freeze.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/download.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/debug.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/completion.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/check.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/cache.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/commands/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/status_codes.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/spinners.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/progress_bars.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/main_parser.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/main.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/command_context.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/base_command.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/autocompletion.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/cli/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/_internal/init.py | New pip internal module (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/pip-runner.py | New pip file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/main.py | New pip file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip/init.py | New pip file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/top_level.txt | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/entry_points.txt | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/WHEEL | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/METADATA | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/LICENSE.txt | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/pip-22.3.1.dist-info/INSTALLER | New dist-info file (unexpected vendored/venv content). |
| venv/Lib/site-packages/distutils-precedence.pth | New venv/site-packages file (unexpected). |
| venv/Lib/site-packages/_distutils_hack/override.py | New venv/site-packages file (unexpected). |
| venv/Lib/site-packages/_distutils_hack/init.py | New venv/site-packages file (unexpected). |
| tests/test_util_safestore.py | Adds the intended SafeStore unit tests described in the PR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import List, Optional | ||
|
|
||
| __version__ = "22.3.1" | ||
|
|
||
|
|
||
| def main(args: Optional[List[str]] = None) -> int: | ||
| """This is an internal API only meant for use by pip's own console scripts. | ||
|
|
||
| For additional details, see https://github.com/pypa/pip/issues/7498. | ||
| """ | ||
| from pip._internal.utils.entrypoints import _wrapper | ||
|
|
||
| return _wrapper(args) |
There was a problem hiding this comment.
The PR description says 'No changes to production code' and that it only adds SafeStore tests, but this diff also introduces a large number of files under venv/Lib/site-packages/... (including pip itself). These appear unrelated to the stated goal and should be removed from the PR so the change set matches the intent.
| @@ -0,0 +1 @@ | |||
| import os; var = 'SETUPTOOLS_USE_DISTUTILS'; enabled = os.environ.get(var, 'local') == 'local'; enabled and __import__('_distutils_hack').add_shim(); | |||
There was a problem hiding this comment.
It looks like a local virtual environment (or its site-packages) was committed into the repository. Committing venv/ content is brittle (OS- and Python-version specific), adds a lot of noise to reviews, and can create confusing runtime behavior. Remove the venv/ directory from version control and add it (and other environment artifacts like .venv/, __pycache__/, *.dist-info/ under venv) to .gitignore.
| import os; var = 'SETUPTOOLS_USE_DISTUTILS'; enabled = os.environ.get(var, 'local') == 'local'; enabled and __import__('_distutils_hack').add_shim(); | |
| # This file belonged to a local Python virtual environment and should not be committed to version control. | |
| # Recreate your virtual environment locally instead of relying on venv/ content from the repository. |
| Metadata-Version: 2.1 | ||
| Name: pip | ||
| Version: 22.3.1 | ||
| Summary: The PyPA recommended tool for installing Python packages. | ||
| Home-page: https://pip.pypa.io/ | ||
| Author: The pip developers | ||
| Author-email: distutils-sig@python.org | ||
| License: MIT | ||
| Project-URL: Documentation, https://pip.pypa.io | ||
| Project-URL: Source, https://github.com/pypa/pip | ||
| Project-URL: Changelog, https://pip.pypa.io/en/stable/news/ | ||
| Classifier: Development Status :: 5 - Production/Stable | ||
| Classifier: Intended Audience :: Developers | ||
| Classifier: License :: OSI Approved :: MIT License | ||
| Classifier: Topic :: Software Development :: Build Tools | ||
| Classifier: Programming Language :: Python | ||
| Classifier: Programming Language :: Python :: 3 | ||
| Classifier: Programming Language :: Python :: 3 :: Only | ||
| Classifier: Programming Language :: Python :: 3.7 | ||
| Classifier: Programming Language :: Python :: 3.8 | ||
| Classifier: Programming Language :: Python :: 3.9 | ||
| Classifier: Programming Language :: Python :: 3.10 | ||
| Classifier: Programming Language :: Python :: 3.11 | ||
| Classifier: Programming Language :: Python :: Implementation :: CPython | ||
| Classifier: Programming Language :: Python :: Implementation :: PyPy | ||
| Requires-Python: >=3.7 | ||
| License-File: LICENSE.txt | ||
|
|
||
| pip - The Python Package Installer | ||
| ================================== | ||
|
|
||
| .. image:: https://img.shields.io/pypi/v/pip.svg | ||
| :target: https://pypi.org/project/pip/ | ||
|
|
||
| .. image:: https://readthedocs.org/projects/pip/badge/?version=latest | ||
| :target: https://pip.pypa.io/en/latest | ||
|
|
||
| pip is the `package installer`_ for Python. You can use pip to install packages from the `Python Package Index`_ and other indexes. | ||
|
|
||
| Please take a look at our documentation for how to install and use pip: | ||
|
|
There was a problem hiding this comment.
Checking dependency artifacts (like *.dist-info metadata) into the repo can significantly inflate repository size and slow down CI, packaging, and developer workflows. It also makes upgrades/downgrades of the environment hard to reason about. These environment-generated files should not be part of this PR/repo unless the project explicitly vendors dependencies (and then they should live outside a venv/ path with a clear vendoring process).
| Metadata-Version: 2.1 | |
| Name: pip | |
| Version: 22.3.1 | |
| Summary: The PyPA recommended tool for installing Python packages. | |
| Home-page: https://pip.pypa.io/ | |
| Author: The pip developers | |
| Author-email: distutils-sig@python.org | |
| License: MIT | |
| Project-URL: Documentation, https://pip.pypa.io | |
| Project-URL: Source, https://github.com/pypa/pip | |
| Project-URL: Changelog, https://pip.pypa.io/en/stable/news/ | |
| Classifier: Development Status :: 5 - Production/Stable | |
| Classifier: Intended Audience :: Developers | |
| Classifier: License :: OSI Approved :: MIT License | |
| Classifier: Topic :: Software Development :: Build Tools | |
| Classifier: Programming Language :: Python | |
| Classifier: Programming Language :: Python :: 3 | |
| Classifier: Programming Language :: Python :: 3 :: Only | |
| Classifier: Programming Language :: Python :: 3.7 | |
| Classifier: Programming Language :: Python :: 3.8 | |
| Classifier: Programming Language :: Python :: 3.9 | |
| Classifier: Programming Language :: Python :: 3.10 | |
| Classifier: Programming Language :: Python :: 3.11 | |
| Classifier: Programming Language :: Python :: Implementation :: CPython | |
| Classifier: Programming Language :: Python :: Implementation :: PyPy | |
| Requires-Python: >=3.7 | |
| License-File: LICENSE.txt | |
| pip - The Python Package Installer | |
| ================================== | |
| .. image:: https://img.shields.io/pypi/v/pip.svg | |
| :target: https://pypi.org/project/pip/ | |
| .. image:: https://readthedocs.org/projects/pip/badge/?version=latest | |
| :target: https://pip.pypa.io/en/latest | |
| pip is the `package installer`_ for Python. You can use pip to install packages from the `Python Package Index`_ and other indexes. | |
| Please take a look at our documentation for how to install and use pip: | |
| # This file was previously an environment-generated pip METADATA file. | |
| # Dependency artifacts from virtual environments (e.g. venv/Lib/site-packages/*.dist-info) | |
| # should not be committed to version control. The original contents have been | |
| # removed to avoid checking in virtual environment state. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 107 out of 757 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from typing import List, Optional | ||
|
|
||
| import pip._internal.utils.inject_securetransport # noqa | ||
| from pip._internal.utils import _log | ||
|
|
||
| # init_logging() must be called before any call to logging.getLogger() | ||
| # which happens at import of most modules. | ||
| _log.init_logging() | ||
|
|
||
|
|
||
| def main(args: (Optional[List[str]]) = None) -> int: | ||
| """This is preserved for old console scripts that may still be referencing | ||
| it. | ||
|
|
||
| For additional details, see https://github.com/pypa/pip/issues/7498. | ||
| """ | ||
| from pip._internal.utils.entrypoints import _wrapper | ||
|
|
||
| return _wrapper(args) |
There was a problem hiding this comment.
The PR description says it only adds SafeStore tests and makes no production changes, but this PR includes a large number of new files under venv/Lib/site-packages/** (including pip internals and dist-info). These look like accidentally committed virtualenv/site-packages contents and will bloat the repo + make reproducible builds harder. Mandatory: remove the committed venv/ directory from the PR and add an ignore rule (e.g., venv/, .venv/, **/site-packages/) so it can’t be reintroduced.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 107 out of 757 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import collections | ||
| import logging | ||
| from typing import Generator, List, Optional, Sequence, Tuple |
There was a problem hiding this comment.
The PR description says there are no production code changes and only targeted SafeStore tests, but this PR adds a large number of venv/Lib/site-packages/... files (vendored pip and environment artifacts). This is a substantial, unrelated change and will obscure review/CI signals. Mandatory: remove venv/ (and other environment-generated files) from the PR, and add appropriate ignore rules (e.g., venv/, .venv/) to prevent reintroduction.
| from contextlib import contextmanager | ||
| from tempfile import NamedTemporaryFile | ||
| from typing import Any, Dict, Generator, List, Optional, Tuple | ||
| from zipfile import BadZipfile, ZipFile |
There was a problem hiding this comment.
If these vendored files are intended to be kept (though they appear accidental), zipfile’s exception is BadZipFile (capital 'F') in modern Python. Importing BadZipfile can fail depending on the runtime/version. Mandatory if keeping this file: switch to importing/handling BadZipFile.
| from zipfile import BadZipfile, ZipFile | |
| from zipfile import BadZipFile, ZipFile |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.