Burn down the invalid-argument-type diagnostics - #830
Conversation
from dascore.units import m gave Unknown, which silently absorbed every check downstream. A name reaching __getattr__ is always a non-empty identifier, so it resolves to a quantity or raises.
The coord map type was wrong in three separate ways, all of which ty flagged as callers failing to match it: - A tuple key was allowed, but get_coord_manager rejects any key that is not a dimension name, so no caller could ever use one. - Only ndarray was allowed as data, but a list or range works and both the dispersion and taup transforms pass one. - dict was used rather than Mapping, so a caller holding a narrower value type did not match even when every value was valid. Patch.new declared its own version of this type; it now shares the alias. A bare tuple stays reserved for the (dimension, data) form. Patch.drop_coords advertised Collection[str], which never worked: a list or set raises TypeError on the set intersection and a tuple silently drops nothing. It takes plain strings, as every caller already passes. _scan_payload_to_summary defaulted source_format and source_version to None, which PatchSummary's before-validator maps to "". Default them to the string the model stores so the annotation matches the field.
FrozenDict.new rebuilt the mapping with `self.__class__(**contents)`, which raises `TypeError: keywords must be strings` for any key that is not a valid identifier. It now passes the mapping positionally. pydantic.validate_call declares a ConfigDict, so build one rather than a plain dict.
is_netcdf4_file and get_cf_version declared h5py.File but receive the managed handle a FiberIO caster produces, and their tests already pass plain duck types. Both read only `attrs`, so that is what they now ask for. get_patch_names forwards its input straight to scan_to_df, so it accepts everything that scans -- including the list of patches the DASDAE writer hands it.
patch_function handles the bare-decorator form by re-entering itself, so required_dims is only ever a callable on that path. Doing the check before the wrapper is built rather than after leaves the rest of the function seeing the tuple of dimension names it actually gets. The assembly loop's first pass sets axis and dims alongside buffer; assert all three rather than just the one.
_KEEP was a bare object(), so _view's order and ids parameters were inferred as object and neither could be passed on to the constructor. A dedicated class carries the same meaning and lets an isinstance check narrow the parameter back to the spec type.
|
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: 24 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 (9)
📝 WalkthroughWalkthroughChangesThe change broadens coordinate input contracts, updates structural type annotations, improves patch decorator handling, normalizes I/O defaults, and adds utility correctness checks. API contract refinements
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 #830 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17916 17924 +8
=========================================
+ Hits 17916 17924 +8
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:
|
Both Patch.drop_coords and CoordManager.drop_coords advertised Collection[str] but no collection form worked: a list or set raised TypeError on the set intersection and a tuple silently dropped nothing. iterate was applied to the varargs tuple as a whole, which is already iterable, so anything passed as a sequence survived as a single unhashable element. Applying it per argument flattens as intended and makes the documented signature true at both levels.
CoordManagerInput claimed more than it delivered. Its Sequence[Any] arm subsumed every tuple, so the "bare tuple is reserved for the (dim, data) form" comment was false and the nested tuple arm rejected nothing, while the union still turned away int and Quantity values (partial coords) that first-party callers pass. It now matches the Patch constructor's coords. get_patch_names referenced io.core.ScanInput behind TYPE_CHECKING. The import cycle is real, but the annotation then failed to resolve at runtime, and the docs renderer falls back all-or-nothing to raw strings for a signature when any name is undefined. Spelled out instead. drop_coords accepts any iterable, not just a sized collection, since iterate branches on Iterable; a generator worked but did not type check. The drop_coords tests only asserted the named coords were gone, so an implementation that dropped every coord passed them. They now compare against the bare-name call.
1 participant
Description
Continues the ty burn-down.
invalid-argument-typegoes from 34 to 18 indascore/; the rules already gating CI stay at zero throughout.The point of this rule is that it is the one that checks callers against what a function claims to accept. Turning it on is also what makes bringing
tests/into ty's scope worthwhile — with the rule ignored, addingtests/reports zero of these, so the test suite cannot act as an oracle for public API annotations until this reaches zero.Most of what follows is not annotation tidying. In each case ty was flagging a caller because the annotation was false, and the interesting part was which side was wrong.
Two latent bugs
FrozenDict.newcould not round-trip non-string keys. It rebuilt the mapping withself.__class__(**contents), so any key that is not a valid identifier raisedTypeError: keywords must be strings.FrozenDict({1: "a"}).new()reproduces it. Now passed positionally. Test added.drop_coordsadvertisedCollection[str], but no collection form worked. A list or set raisedTypeErroron the set intersection, and a tuple silently dropped nothing:Patch.drop_coordsandCoordManager.drop_coordsnow flatten with the existingiterateutility, so a name, a sequence of names, or a mix of both works at either level. The bug was thatiteratewas applied to the varargs tuple as a whole, which is already iterable — so a sequence passed in survived as a single unhashable element. It is now applied per argument. Tests cover list/tuple/set at both levels, mixed args, and a dimension named inside a sequence still raising.The coord-map input type was wrong three ways
CoordManagerInputandPatch.new's private copy of it disagreed with each other and with the runtime:get_coord_managerrejects any key that is not a dimension name, so no caller could ever use one;ndarraywas allowed as data, but a list or range works — and both the dispersion and taup transforms pass one;dictrather thanMapping, so a caller holding a narrower value type did not match even when every value was valid.Patch.newnow shares the alias instead of maintaining its own. A bare tuple stays reserved for the(dimension, data)form, which is whySequence[Any]rather than something looser.Annotations narrower than their callers
is_netcdf4_file/get_cf_versiondeclaredh5py.Filebut receive the managed handle a FiberIO caster produces — and their existing tests already pass plain duck types. Both read onlyattrs, so that is what they ask for now.get_patch_namesforwards straight toscan_to_df, so it accepts everything that scans, including the list of patches the DASDAE writer hands it.Narrowings made visible
patch_functionhandles the bare-decorator form by re-entering itself. Doing that check before the wrapper is built rather than after leaves the rest of the function seeingrequired_dimsas the tuple it actually is. All three decorator forms verified unchanged.axisanddimsalongsidebuffer; all three are now asserted rather than just the one._KEEPwas a bareobject(), so_view'sorderandidswere inferred asobjectand could not be passed on. A dedicated class carries the same meaning and narrows.dascore.units.__getattr__was untyped, sofrom dascore.units import minferredUnknownand silently absorbed every downstream check on it. NowQuantity.What was deliberately left
Three clusters were investigated and rejected, rather than forced:
to_int/to_float(2 diagnostics). These aresingledispatchgenerics declared-> np.ndarray, which is false. Widening to the true union pushes the count up to 34, because callers genuinely rely on the input-dependent return type.@overloadis the right tool, but applying it makes ty lose.registerentirely (27 new errors) — the two decorators do not compose. Left alone.memoryview(ndarray),blake2b.update(ndarray), and1 / coord.unitsare all correct at runtime; the stubs are imprecise.FrozenDict.__getitem__(1 diagnostic).**kwargsforcesstrkeys, so_dictreally isdict[K | str, V], andSupportsKeysAndGetItemis invariant in its key. Not expressible without a cast that buys nothing.Changelog
Patch.drop_coordsandCoordManager.drop_coordsaccept a sequence of names as well as bare names; a list or set previously raisedTypeErrorand a tuple or generator was silently ignored, and a name that is a dimension raisesParameterError.Checklist
I have (if applicable):
Summary by CodeRabbit
Bug Fixes
Improvements
Tests