Enable ty's invalid-argument-type rule - #836
Conversation
CoordManager.__rich__ declared -> str but returns Text.assemble(...). The filesystem generators declared a SendType of str, but None is an explicitly supported send value: the indexer primes them with send(None) and both sub-generators test `if signal is not None`. All three change together because yield from delegates the send value.
_get_transformed_coord hand-rolled `1 / coord.units`; invert_quantity already does exactly this, including the None/NaN guard, and is used for the same expression in transform/fourier.py and proc/filter.py. The directory walker built Path(candidate) from a yield type that includes UPath. UPath is not os.PathLike unless it resolved to a local path, so that would raise TypeError for a remote resource; coerce_to_local_path handles it and also resolves file:// URIs.
A constrained TypeVar was the wrong tool: it demands an exact match, so callers passing None (returned unchanged, and relied upon) or a value the checker only knows as object were rejected outright. The -> numeric promise was false anyway, since an int in yields a float out. Widening it also makes the unsupported-operator suppression unnecessary, which has to go with it or unused-ignore-comment fires. _maybe_transform_units rebound filt inside the try, so the except branch was typed as the union of both states.
ndarray declares __buffer__ only for Python 3.12+, so a checker resolving this project's 3.11 floor cannot see it. Going through .data, which numpy types as a memoryview, says the same thing at runtime -- it is the same zero-copy object, and digests are byte-identical for C-contiguous, Fortran-ordered, strided datetime64, timedelta64, and empty arrays. FrozenDict._dict inferred as dict[str, Unknown] because **kwargs makes the checker pick typeshed's str-keyed dict overload, so lookups by K failed. Declaring it and casting in the constructor says what the class holds. new() merges in a dict literal now, since update's overloads all require str keys.
_UNSET was a bare object(), so target_units collapsed to object and the checker could not see the str | None it actually holds. An enum member narrows on an is comparison and keeps the identity semantics the three comparison sites rely on. The distinction is load-bearing -- None means the coordinate is unitless, which cannot answer a query carrying units -- so it now has a test of its own. _coord_record_from_row narrowed only the min, leaving float(None) on the max reachable as far as the checker could tell. Every producer writes min and max together, so this asserts that rather than inventing behaviour for a state no caller can construct.
PatchSummary is not a Mapping at runtime, but it is not final either, so a checker has to assume a subclass could be both and leaves the intersection in the Mapping branch. Testing for it first narrows it out. No input can change branch, since the intersection is empty. dict() on a TypedDict erases its value types, so attrs came out as object. _validate_scan_payload has already raised unless every key holds what ScanPayload declares, so the cast states what is known there.
Both bases claimed to return np.ndarray while actually returning an int, a float, or a Series depending on what was passed, so every registered implementation with an honest return annotation was rejected. Overloads say it properly -- a Series stays a Series, other sequences become arrays, everything else is a scalar -- but stacking @overload on a singledispatch loses .register, so the dispatchers move to _to_int and _to_float and the public names become wrappers. Measured at ~15 ns per scalar call and ~30 ns per array call. to_int's scalar overload is int | np.integer | float, not int: time-like values convert through numpy and yield np.int64, and null yields NaN. The to_float fallback also called float() on anything unregistered, which pd.Timestamp does not support; it is reachable only for types float() already handles, and there is now a test pinning that.
WARNING_ACTIONS advertised "all", which Python only began accepting in 3.14; on 3.11 and 3.12 it trips an assertion and on 3.13 it raises ValueError, so three of the four supported interpreters reject it. Nothing passes it, and it aliases "always". With that the rule reaches zero on every supported Python version, so it no longer needs to be ignored.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates typing contracts and runtime compatibility across time conversion, scanning, indexing, units, utilities, visualization, documentation, and type-checker configuration. It adds edge-case tests for unit handling and numeric conversion. ChangesTyping and runtime compatibility
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/io/index/planned.py`:
- Around line 104-108: Replace the assert on hi in the relevant
envelope-processing logic with explicit null validation that rejects both None
and pandas NaN/NaT values before numeric or datetime conversion, raising the
appropriate runtime exception. Preserve the existing handling for valid maxima
and the special string-envelope case.
In `@dascore/units.py`:
- Around line 195-209: Update convert_units to return data unchanged immediately
when data is None, before applying any conversion factors or using from_units.
Preserve the existing conversion behavior for non-None inputs.
In `@docs/changelog.qmd`:
- Line 7: Update the changelog entry describing to_float so it explicitly
documents the new fallback behavior: unsupported values such as Decimal("3") and
"1.5" are converted through float(). Replace the statement that runtime behavior
is unchanged with wording that distinguishes this public behavior change from
the unchanged existing conversions.
🪄 Autofix
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: 73b528d7-8da4-4b4b-8763-7be9c814abc0
📒 Files selected for processing (17)
dascore/constants.pydascore/core/coordmanager.pydascore/io/core.pydascore/io/index/indexer.pydascore/io/index/planned.pydascore/io/index/query.pydascore/proc/filter.pydascore/units.pydascore/utils/array.pydascore/utils/mapping.pydascore/utils/misc.pydascore/utils/time.pydascore/viz/spectrogram.pydocs/changelog.qmdpyproject.tomltests/test_io/test_index/test_index_edge_cases.pytests/test_utils/test_time.py
| # Only the str envelope above represents a missing max. Every producer | ||
| # writes {name}_min and {name}_max together -- _output_records feeds | ||
| # whole dataframe rows (a missing value is NaN, not None) and the aux | ||
| # info dict always sets both -- so a max cannot be absent past here. | ||
| assert hi is not None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject null maxima before conversion.
Line 108 checks only hi is not None. Pandas row dictionaries represent missing values as NaN, and NaN is not None, so the assertion passes. The numeric branch can then store nan, while the datetime branch can propagate NaT into the coordinate record.
Use an explicit null check and exception. Do not use assert for this runtime validation.
Proposed fix
- assert hi is not None
+ if hi is None or pd.isnull(hi):
+ raise ValueError(
+ f"Non-string coordinate {name!r} requires a non-null maximum."
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Only the str envelope above represents a missing max. Every producer | |
| # writes {name}_min and {name}_max together -- _output_records feeds | |
| # whole dataframe rows (a missing value is NaN, not None) and the aux | |
| # info dict always sets both -- so a max cannot be absent past here. | |
| assert hi is not None | |
| # Only the str envelope above represents a missing max. Every producer | |
| # writes {name}_min and {name}_max together -- _output_records feeds | |
| # whole dataframe rows (a missing value is NaN, not None) and the aux | |
| # info dict always sets both -- so a max cannot be absent past here. | |
| if hi is None or pd.isnull(hi): | |
| raise ValueError( | |
| f"Non-string coordinate {name!r} requires a non-null maximum." | |
| ) |
🤖 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/io/index/planned.py` around lines 104 - 108, Replace the assert on hi
in the relevant envelope-processing logic with explicit null validation that
rejects both None and pandas NaN/NaT values before numeric or datetime
conversion, raising the appropriate runtime exception. Preserve the existing
handling for valid maxima and the special string-envelope case.
|
|
||
| ## Unreleased API Changes | ||
|
|
||
| - `dascore.utils.time.to_int` and `to_float` are now overloaded wrappers over private `singledispatch` implementations, so a `Series` input is typed as returning a `Series` and an array as returning an array. Their runtime behaviour is unchanged, but `to_int.register(...)` and `to_float.register(...)` no longer exist; register new implementations on `_to_int` / `_to_float` instead. `convert_units` no longer declares a constrained `numeric` type variable — it accepted (and still accepts) `None`, quantities, and numpy scalars, none of which that variable admitted. `WARNING_ACTIONS` no longer lists `"all"`, which Python only began accepting in 3.14 and which raises on the 3.11–3.13 interpreters DASCore also supports; use `"always"`, which it aliases. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the new to_float fallback behavior.
Line 7 says that runtime behavior is unchanged. The new fallback accepts values such as Decimal("3") and "1.5" through float(). State this public behavior change in the changelog.
🤖 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 `@docs/changelog.qmd` at line 7, Update the changelog entry describing to_float
so it explicitly documents the new fallback behavior: unsupported values such as
Decimal("3") and "1.5" are converted through float(). Replace the statement that
runtime behavior is unchanged with wording that distinguishes this public
behavior change from the unchanged existing conversions.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #836 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17936 17945 +9
=========================================
+ Hits 17936 17945 +9
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:
|
|
✅ Documentation built: |
# Conflicts: # dascore/utils/time.py # docs/changelog.qmd
An @overload set is only verified against its implementation when the implementation's own return is annotated; without it the return is untyped and satisfies anything, so a wrong overload passes silently and is then believed at every call site. With the return declared, ty reports invalid-overload for an overload whose return is not assignable to it -- confirmed by declaring to_float(pd.Series) -> str and watching it fail.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/utils/time.py (1)
367-368: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the catch-all
Anyoverloads.These overloads make every argument valid for
to_int/to_float, while the runtime implementation still rejects unsupported inputs raisesNotImplementedError. Use explicit supported-input overloads for these functions at lines 388-397 and 495-508.🤖 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/utils/time.py` around lines 367 - 368, Replace the catch-all Any overloads for to_int and to_float with overloads that enumerate only their supported input types, matching the runtime implementations’ accepted values. Keep unsupported arguments excluded from static typing so they remain rejected by the existing NotImplementedError path.
🤖 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.
Outside diff comments:
In `@dascore/utils/time.py`:
- Around line 367-368: Replace the catch-all Any overloads for to_int and
to_float with overloads that enumerate only their supported input types,
matching the runtime implementations’ accepted values. Keep unsupported
arguments excluded from static typing so they remain rejected by the existing
NotImplementedError path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c72455a2-e516-4cdc-bd8f-2cd7bd4f64e1
📒 Files selected for processing (1)
dascore/utils/time.py
None only survives the from_units is None path, where the data is returned untouched; with real conversion factors it raises.
1 participant
Description
Takes ty's
invalid-argument-typefrom 18 to 0 and removes it from[tool.ty.rules], so the rule now gates CI. Follows #830, which took it 34 → 18.invalid-return-typestays ignored and is the next PR — it fell from 31 to 26 as a side effect of the fixes here.No suppression comments were needed; one existing suppression became unnecessary and was removed.
Bugs found along the way
These are real defects, not annotation noise:
WARNING_ACTIONSadvertised"all", which Python only began accepting in 3.14. On 3.11/3.12 it trips an assertion and on 3.13 it raisesValueError— three of the four supported interpreters reject it. Nothing passed it, and it aliases"always".Path(candidate)in the directory walker was built from a yield type includingUPath, which is notos.PathLikeunless it resolved to a local path.coerce_to_local_pathhandles it and also resolvesfile://URIs.to_float's fallback calledfloat()on anything unregistered, whichpd.Timestampdoes not support — reachable only because registration shadows it. Now pinned by a test._coord_record_from_rownarrowed only the min, leavingfloat(None)on the max reachable as far as a checker could tell. Every producer writes min and max together, so that is asserted rather than given invented behaviour.CoordManager.__rich__declared-> strbut returnsText;to_int/to_floatdeclared-> np.ndarraywhile returningint,float, orSeries; the filesystem generators declared aSendTypeofstralthough the indexer primes them withsend(None).Notable changes
to_int/to_floatbecome overloaded wrappers over private_to_int/_to_floatdispatchers, so aSeriesin is typed as aSeriesout and an array as an array. Stacking@overloadon asingledispatchloses.register, hence the split. Measured at ~15 ns per scalar call and ~30 ns per array call.to_int.register/to_float.registerno longer exist — register on the private dispatchers.convert_unitsdrops its constrainednumericTypeVar, which rejectedNone, quantities, and numpy scalars that it accepts and returns. That also makes its# ty: ignore[unsupported-operator]unnecessary.hash_arraygoes throughndarray.datainstead ofmemoryview(arr). Same zero-copy object; digests verified byte-identical for C-contiguous, Fortran-ordered, strided datetime64, timedelta64, and empty arrays.invert_quantityandcoerce_to_local_path.Verification
Checked on every supported Python version, since ty resolves stdlib stubs per version and 3.14 alone caught the
WARNING_ACTIONSbug:Full suite 8120 passed / 251 skipped / 2 xfailed;
pre-commit run --all-filesgreen.Changelog
to_intandto_floatare overloaded wrappers over privatesingledispatchimplementations, soto_int.register(...)no longer exists — register on_to_int/_to_floatinstead. Runtime behaviour is unchanged.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Documentation