Skip to content

remove usage of the _deprecate_positional_args decorator, and tests for its behavior #3225

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

Merged
merged 9 commits into from
Jul 14, 2025

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jul 10, 2025

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/user-guide/*.rst
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@d-v-b d-v-b added this to the 3.1.0 milestone Jul 10, 2025
@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Jul 10, 2025
@d-v-b d-v-b requested a review from dstansby July 10, 2025 17:13
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.61%. Comparing base (7154f98) to head (77a9b16).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3225      +/-   ##
==========================================
- Coverage   94.76%   94.61%   -0.15%     
==========================================
  Files          78       78              
  Lines        8672     8643      -29     
==========================================
- Hits         8218     8178      -40     
- Misses        454      465      +11     
Files with missing lines Coverage Δ
src/zarr/api/synchronous.py 92.85% <ø> (-1.81%) ⬇️
src/zarr/core/array.py 98.37% <ø> (-0.15%) ⬇️
src/zarr/core/group.py 94.81% <ø> (-0.07%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 11, 2025

there's a reduction in coverage, but the previously covered lines of code were not actually being tested for correctness -- they were just being run to check the behavior of the positional args decorator. So I think we can disregard the coverage reduction here.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 11, 2025

this failure is due to recent changes to our hypothesis tests, presumably revealing a real bug, but likely one unrelated to this PR (cc @dcherian)

@d-v-b d-v-b mentioned this pull request Jul 13, 2025
23 tasks
@d-v-b d-v-b merged commit b267ff6 into zarr-developers:main Jul 14, 2025
26 of 28 checks passed
@d-v-b d-v-b deleted the keyword-only branch July 14, 2025 16:08
@dstansby
Copy link
Contributor

This breaks the hypothesis tests and pre-commit...

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

pre-commit is failing, and hypothesis is also reporting a failed test, but not because of this PR

@dstansby
Copy link
Contributor

There were pre-existing pre-commit errors before this PR, but this introduced more. And I'm pretty sure this broke the hypothesis tests? They were working before this PR was merged: https://github.com/zarr-developers/zarr-python/actions/runs/16271528357/job/45940137517

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

compare the changes in this PR (removing a decorator that only raised a warning, and some tests for that decorator) with the pre-commit errors (type-checking failures in obstore and hypothesis tests). I don't see how they are related other than by timing. We should just fix the pre-commit errors (by correcting type hints as needed).

As for the hypothesis test failure, I don't see how that could be related to the changes in this PR since it pertains to deleting groups from a local file system-backed store. we should of course also fix this. but it seems very unlikely to be caused by the changes in this PR.

@dstansby
Copy link
Contributor

I don't see how they are related other than by timing.

If I run pre-commit on the main commit before this was merged (2f64d67), I get 7 errors. If I run it after this was merged (b267ff6) I get 14 errors. I think that's a pretty clear smoking gun that the new errors were caused by this PR, and this PR changed the acceptable call signatures of lots of functions/methods, I don't think it's that unlikely. For the same reason I don't think it's unlikely that the hypothesis failures were caused by this PR.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 14, 2025

Good detective work! Do you want to work on fixing the mypy problems? And @dcherian do you have any idea what's causing the hypothesis test failures?

d-v-b added a commit to d-v-b/zarr-python that referenced this pull request Jul 14, 2025
d-v-b added a commit that referenced this pull request Jul 15, 2025
* complete partial release note

* build release notes

* add release note for #3225

* clarify keyword only changes
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.

3 participants