Enable ty's not-subscriptable and no-matching-overload rules - #800
Merged
Conversation
_required_resource_type read the marker through an unconditional cast, which had no runtime behavior and turned an unwrapped method into an AttributeError rather than None. Meanwhile _get_fiber_io_and_req_type did the same lookup a second way with getattr. One concept, one idiom.
Both rules go to zero and come off the ignore list. BaseCoord.shape was typed `tuple[int, ...] | None` but every coord has one: each subclass derives it in a before-validator, and the field validator already normalized a missing value to (). The None was only a placeholder, so the four `self.shape[0]` sites read as subscripting None. Making shape required instead traded those four errors for 21 missing-argument reports, since the before-validators are invisible to type checkers, so the annotation keeps a default. The remaining fixes: - _get_dx_or_spacing_and_axes is overloaded on require_evenly_spaced. With it set, get_coord requires an evenly sampled coord, so every returned value is a scalar spacing rather than an array of values; callers no longer see the union. - NUMPY_TIME_UNIT_MAPPING declares its literal unit codes, which is what numpy's unit parameter accepts. - The degenerate-array branches in _array_to_datetime64 and _array_to_timedelta64 unpack with `[()]` instead of rebuilding the scalar through np.datetime64/np.timedelta64, which drops the duplicated astype in each. _array_to_timedelta64 was also annotated `-> np.datetime64`; it returns timedeltas. - maybe_mem_map checks for a name before mapping instead of handing np.memmap a None it is guaranteed to reject. The fallback now also covers a name that exists but cannot be mapped, which gets a test. - Spool.split branches on count rather than relying on np.ceil to absorb a None. _spool_map was passing it a float size, so it rounds up itself and split's `int` annotation is now true. - patch_function calls pydantic.validate_call as the decorator factory it is rather than passing func and config together. - _get_band_attr_data_type compares against the first mapped band once instead of scanning for None separately. - The spool query coerces with pd.Timestamp.to_datetime64. The split-by-count path had no test; the one named for it split by size.
CoordPartial redeclares shape without a default. It is the one coord which cannot re-derive its shape on the way back from a model_dump(exclude_defaults=True) -- the array coords rebuild it from values, CoordRange from start/stop/step, CoordSegmented from its segments -- so with () as the base default a dimensionless partial coord lost its only defining field on the round trip through CoordManager. Spool.split keeps ceiling a non-integral size rather than slicing with whatever it was handed.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request strengthens type annotations, updates type-checker configuration, and adjusts coordinate, spool, I/O, time, and memory-mapping behavior. It adds regression coverage for dimensionless coordinates, spool splitting, and unsupported memory-mapped files. ChangesTyping and runtime robustness updates
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #800 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17824 17829 +5
=========================================
+ Hits 17824 17829 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_core/test_spool.py (1)
606-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the non-integral
sizepath.The production change rounds non-integral sizes, but Line 608 passes
size=2. Use a fractional value such assize=1.5and keep the expected lengths2and1.Proposed test change
- split = list(random_spool.split(size=2)) + split = list(random_spool.split(size=1.5))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_spool.py` around lines 606 - 611, Update test_uneven_size to call random_spool.split with a fractional size such as 1.5, while preserving the existing assertions that the two resulting spools have lengths 2 and 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dascore/core/spool.py`:
- Around line 587-591: Validate the split parameters before the step calculation
in the affected spool splitting method: reject any provided count or size that
is less than or equal to zero by raising ParameterError. Ensure this validation
occurs before computing step so count=0, size=0, and negative values cannot
produce invalid or non-advancing iteration.
In `@dascore/utils/misc.py`:
- Around line 1015-1026: Update maybe_mem_map() to include OSError, including
FileNotFoundError, in the exceptions caught around np.memmap() so unreadable or
unmappable named file-like objects use the existing fid.seek(0) and
np.frombuffer fallback. Add a regression test covering a readable named
file-like object whose path cannot be mapped.
In `@dascore/utils/time.py`:
- Around line 245-254: Update _array_to_timedelta64 to use array.size instead of
len(array) when checking for empty inputs, allowing zero-dimensional arrays to
proceed without TypeError. Ensure zero-dimensional np.datetime64 inputs are
converted through the existing datetime path and unpacked to a scalar, matching
the degenerate timedelta result behavior.
---
Nitpick comments:
In `@tests/test_core/test_spool.py`:
- Around line 606-611: Update test_uneven_size to call random_spool.split with a
fractional size such as 1.5, while preserving the existing assertions that the
two resulting spools have lengths 2 and 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e8f7f03-e7d7-43c5-9e96-af0b135f3594
📒 Files selected for processing (13)
dascore/constants.pydascore/core/coords.pydascore/core/spool.pydascore/io/core.pydascore/io/index/query.pydascore/io/sintela/protobuf_utils.pydascore/utils/misc.pydascore/utils/patch.pydascore/utils/time.pypyproject.tomltests/test_core/test_coords.pytests/test_core/test_spool.pytests/test_utils/test_misc.py
Comment on lines
+587
to
+591
| if count is not None: | ||
| step = int(np.ceil(len(self) / count)) | ||
| else: | ||
| assert size is not None # the check above sets exactly one of them | ||
| step = int(np.ceil(size)) # tolerate a non-integral size |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Reject non-positive split parameters before computing step.
Line 588 divides by count, so count=0 raises ZeroDivisionError. Line 591 can produce step=0 for size=0. Lines 592-594 then never advance. Negative values can also make the generator run forever. Validate that both parameters are greater than zero and raise ParameterError.
Proposed fix
if not ((count is not None) ^ (size is not None)):
msg = "Spool.split requires either spool_count or spool_size."
raise ParameterError(msg)
+ if count is not None and count <= 0:
+ raise ParameterError("count must be greater than zero.")
+ if size is not None and size <= 0:
+ raise ParameterError("size must be greater than zero.")
start = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dascore/core/spool.py` around lines 587 - 591, Validate the split parameters
before the step calculation in the affected spool splitting method: reject any
provided count or size that is less than or equal to zero by raising
ParameterError. Ensure this validation occurs before computing step so count=0,
size=0, and negative values cannot produce invalid or non-advancing iteration.
Contributor
Author
There was a problem hiding this comment.
Real, but out of scope here and pre-existing: size=0 and negative sizes have produced a non-terminating generator since the method was written, and this PR did not touch that path. The only thing that changed is count=0, which now raises ZeroDivisionError where it previously raised TypeError — both unhelpful. Guarding the parameters means adding new ParameterError paths to a public API, which wants its own PR and tests rather than riding along in a type-checking change. Tracking it separately.
Contributor
There was a problem hiding this comment.
@d-chambers, acknowledged. This PR should remain focused on the type-checking changes.
A follow-up change can define the public behavior for non-positive count and size, then add the required tests. I will leave this finding unresolved for the separately tracked work.
You are interacting with an AI system.
maybe_mem_map only caught the errors a bad name argument produces, so a name that is a real path numpy still cannot map -- unlinked since the handle was opened, on a filesystem without mmap -- failed a read the handle could have served. The comment already claimed the broader behavior; now the code does too. Also covers the non-integral split size, which nothing exercised: every test passed a whole number, so the ceil could have been dropped unnoticed. Both from review of #800.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_utils/test_misc.py`:
- Around line 769-778: Update test_name_no_longer_on_disk to skip the
open-handle unlink scenario when sys.platform == "win32", before attempting
path.unlink(); preserve the existing assertions and behavior on platforms that
support unlinking an open file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 28044766-36dc-418e-b592-ff3e90543810
📒 Files selected for processing (3)
dascore/utils/misc.pytests/test_core/test_spool.pytests/test_utils/test_misc.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_core/test_spool.py
- dascore/utils/misc.py
Windows will not unlink a file that is still open, so the test raised PermissionError before reaching what it was checking. A name pointing nowhere reaches the same fallback on every platform, and matches the _NamedBytesIO pattern the buffer-size test above it already uses.
coderabbitai
Bot
removed
bug
This was referenced Aug 4, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Third step of the ty burn-down (#795, #796, #797).
not-subscriptableandno-matching-overloadboth go to zero and come off the ignore list, leaving three ignored rules instead of five.The one API question
BaseCoord.shapewas typedtuple[int, ...] | Nonebut every coord has a shape: each subclass derives it in a before-validator, and the field validator already normalized a missing value to(). TheNonewas a placeholder that no constructed coord ever carried, which is why the fourself.shape[0]sites read as subscriptingNone.Making it required instead traded those four errors for 21
missing-argumentreports, since the before-validators that supply it are invisible to type checkers, so the annotation keeps a default.CoordPartialdoes declare it required: it is the one coord which cannot re-derive its shape on the way back frommodel_dump(exclude_defaults=True)— the array coords rebuild it fromvalues,CoordRangefrom start/stop/step,CoordSegmentedfrom its segments — so with()as the base default a dimensionless partial coord would lose its only defining field on the round trip throughCoordManager. That has a regression test.Root-cause fixes
_get_dx_or_spacing_and_axesis overloaded onrequire_evenly_spaced. With it set,get_coordrequires an evenly sampled coord, so every returned value is a scalar spacing rather than an array of values; the two callers that pass it no longer see the union.NUMPY_TIME_UNIT_MAPPINGdeclares its literal unit codes, which is what numpy's unit parameter accepts — notstr._array_to_datetime64and_array_to_timedelta64unpack with[()]instead of rebuilding the scalar throughnp.datetime64/np.timedelta64, which drops the duplicatedastypein each._array_to_timedelta64was also annotated-> np.datetime64; it returns timedeltas, and fixing that removed threeinvalid-return-typereports as well.maybe_mem_mapchecks for a name before mapping instead of handingnp.memmapaNoneit is guaranteed to reject. The fallback now also covers a name that exists but cannot be mapped (an empty file, an fd number), which gets a test.Spool.splitbranches oncountrather than relying onnp.ceilto absorb aNone._spool_mapwas passing it a float size, so it rounds up itself andsplit'sintannotation is now true.patch_functioncallspydantic.validate_callas the decorator factory it is rather than passingfuncandconfigtogether._get_band_attr_data_typecompares against the first mapped band once instead of scanning forNoneseparately.pd.Timestamp.to_datetime64.Carried over from #797
_required_resource_typeread the_required_typemarker through an unconditionalcast, which had no runtime behavior and turned an unwrapped method into anAttributeErrorrather thanNone, while_get_fiber_io_and_req_typedid the same lookup a second way withgetattr. One concept, one idiom, on its own commit.Tests
The split-by-count path had no test — the one named for it split by size. That is now two tests, and the empty-shape round trip and unmappable-file fallback are covered.
The counts comment in
pyproject.tomlis refreshed:invalid-argument-type166,invalid-return-type67,invalid-method-override36. Bringingtests/into scope stays for a later step.Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Refactor
Tests