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

Apply assorted Repo-Review suggestions #1705

Closed

Conversation

DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Mar 12, 2024

Mostly stricter pytest and MyPy settings.

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
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@pep8speaks
Copy link

pep8speaks commented Mar 12, 2024

Hello @DimitriPapadopoulos! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-05-07 15:56:55 UTC

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.99%. Comparing base (270aff1) to head (10f725c).

❗ Current head 10f725c differs from pull request most recent head b109932. Consider uploading reports for the commit b109932 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1705   +/-   ##
=======================================
  Coverage   99.98%   99.99%           
=======================================
  Files          38       38           
  Lines       14667    14580   -87     
=======================================
- Hits        14665    14579   -86     
+ Misses          2        1    -1     
Files Coverage Δ
zarr/_storage/store.py 100.00% <100.00%> (ø)
zarr/_storage/v3.py 100.00% <100.00%> (ø)
zarr/convenience.py 100.00% <100.00%> (ø)
zarr/storage.py 100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review March 13, 2024 08:38
@@ -2497,7 +2505,7 @@ def _cache_value(self, key: Path, value):
value_size = buffer_size(value)
# check size of the value against max size, as if the value itself exceeds max
# size then we are never going to cache it
if self._max_size is None or value_size <= self._max_size:
if self._max_size is None or value_size <= self._max_size: # type: ignore[redundant-expr]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

self._max_size is initialised here:

zarr-python/zarr/storage.py

Lines 2406 to 2408 in f58065b

def __init__(self, store: StoreLike, max_size: int):
self._store: BaseStore = BaseStore._ensure_store(store)
self._max_size = max_size

and here:

zarr-python/zarr/storage.py

Lines 2417 to 2420 in f58065b

def __getstate__(self):
return (
self._store,
self._max_size,

In the first case self._max_size shouldn't be None, but then the typing system cannot prevent giving the max_size parameter the None value. In the second case, without typing, I don't understand why ruff thinks the self._max_size is None check is redundant.

Must have a minversion=, and must be at least 6 (first version to support pyproject.toml configuration).
log_cli_level should be set. This will allow logs to be displayed on failures.
xfail_strict should be set. You can manually specify if a check should be strict when setting each xfail.
--strict-config should be in addopts = [...]. This forces an error if a config setting is misspelled.
--strict-markers should be in addopts = [...]. This forces all markers to be specified in config, avoiding misspellings.
An explicit summary flag like -ra should be in addopts = [...] (print summary of all fails/errors).
Projects should group their updates to avoid extra PRs and stay in sync.
This is now supported by dependabot since June 2023.
Must have "ignore-without-code" in enable_error_code = [...]. This will
force all skips in your project to include the error code, which makes
them more readable, and avoids skipping something unintended.
Must have "redundant-expr" in enable_error_code = [...]. This helps
catch useless lines of code, like checking the same condition twice.
Left operand of "or" is always false  [redundant-expr]
Must have "truthy-bool" in enable_error_code = []. This catches
mistakes in using a value as truthy if it cannot be falsey.
@jhamman
Copy link
Member

jhamman commented Jun 1, 2024

@DimitriPapadopoulos - thanks for moving your focus over to the v3 branch. I'm going to close this in favor of the contributions you've already made there. 🙏

@jhamman jhamman closed this Jun 1, 2024
@DimitriPapadopoulos DimitriPapadopoulos deleted the repo-review branch June 2, 2024 08:10
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.

None yet

4 participants