Correct public signatures that misdescribed their inputs and returns - #842
Conversation
Only None or Ellipsis yields None, but the single signature made every
result Quantity | None, so get_quantity("m") could not be multiplied
without a narrowing check.
The optional-dependency imports are ignored across tests/, matching what dascore/ already does. The pydantic extra=allow attribute reads are ignored in the six files that actually do them, rather than across the suite, so the rule keeps working on the rest. Also narrows some optionals in the index edge cases and moves its hand-rolled attribute patching onto monkeypatch, which restores itself.
febus already defined tuple types admitting ... for an open end; those move to dascore.constants and dc.read now uses them, so the documented (value, ...) form type checks. The dasvader test builds structured arrays from runtime dtypes, which numpy types as float64, so every field-name index needs a cast to be seen as a void array.
Dropping **kwargs, narrowing a parameter to SpoolType and widening a return to bool are all real override incompatibilities: a caller using the base contract would break on these. The dummies ignore their arguments, so the signatures change and the behaviour does not.
The segmented coord tests reach for segment-specific attributes on values the factory declares as BaseCoord; asserting the concrete type first is also a stronger assertion. dc.write is now generic over the path type, so handing it a Path gets a Path back rather than the whole path_types union that #840 widened it to.
Naming a unit always produces a string; only a null input gives None. The heterogeneous dicts splatted into typed constructors infer an object value type, which rejects every field.
Merging a string literal into a dict[str, Any] widens the value type to Any | str, which none of the record fields accept, so the merges are bound to annotated locals (and the repeated rebase is now a helper).
get_filter_units takes None or ... for an open bound and returns None there too, which its annotation denied in both directions. get_coord takes an int for data, meaning a partial coord of that length.
A plain list is accepted for data but ArrayLike does not cover it. The tests reaching for range-specific attributes now assert the concrete coord class first, which is also a stronger assertion.
filter_df returns a Series once any filter applies and a bare array otherwise; it claimed only the array. The reflected comparison and the list-plus-timedelta both run through the operand numpy or dascore owns, so they are written in that order.
dc.utils.downloader.fetch and friends only resolve if something else has already imported the submodule, which is why the checker calls it possibly missing; importing the name directly is also how the rest of the suite spells it.
The source-side signature fixes stand on their own; the scope flip returns with the remaining tests diagnostics.
get_quantity("") and get_quantity_str("") both return None, and the
empty string is how dascore spells "no units", so typing a str input as
always producing a Quantity was false. It would also have hidden a real
error class: an unset unit reaching arithmetic is a bug the optional
return is supposed to surface.
The test is documented as covering number-first and patch-first; making both lines patch-first deleted half of it. int.__lt__ is declared to return bool, so the reflected result is cast instead.
filter_df's docstring named the wrong split: an equality query returns a bare array too, so the contract is now stated as an opaque boolean container. The filesystem tests assert nothing was dropped rather than silently discarding a None. resample divided by get_filter_units' result without checking it, which the widened return exposed; a null period now raises ParameterError instead of surfacing as a NaN conversion.
|
Warning Review limit reached
Next review available in: 48 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 (28)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #842 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17948 17953 +5
=========================================
+ Hits 17948 17953 +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:
|
1 participant
Description
Corrects public signatures that misdescribed what the functions already accepted and returned, and fixes the two real bugs that surfaced along the way. Follows #830, #836 and #840.
These were found by putting
tests/undertylocally. That scope change is not part of this PR —[tool.ty.src]still coversdascore/only, and the remaining test-side diagnostics land in a follow-up. What is here stands on its own.Signature corrections
get_filter_unitsdeclared-> tuple[float, float]but returns(None, 10.0)for an open bound, and acceptedNone/...that its parameters denied. Wrong in both directions.filter_dfdeclared-> np.ndarraybut returns apandas.Seriesfor collection and range queries.get_coordaccepts a plain list fordata, and anintnaming a length, neither of whichArrayLikecovers.dc.read'stimeanddistanceaccept the documented(value, ...)open-range form. The tuple types for this already existed, private to the febus reader; they move todascore.constantsastime_select_type/float_select_typeand febus now aliases them.dc.writeis generic over its path type (_PathTbound topath_types), so aPathin yields aPathout. This repairs a regression from Enable ty's invalid-return-type rule #840, where widening the return to the whole union brokedc.write(...).exists().Bugs fixed
patch.resample(dim=None)divided byget_filter_units' result without checking it, failing later withValueError: cannot convert float NaN to integer. It now raisesParameterErrornaming the dimension. Found by the counterpart review, with a regression test._is_dasvader_jld2and the other Enable ty's invalid-return-type rule #840 fixes are already ondev; nothing here overlaps them.Test improvements
Narrowed optionals (
assert x is not Nonebefore use),assert isinstance(...)before reaching for a coord subclass's attributes,monkeypatch.setattrin place of hand-rolled attribute patching that restored by hand,FiberIOtest subclasses conforming to the base signatures they claim to implement, and submodules imported directly rather than reached through their parent package.What did not survive review
I had overloaded
get_quantity/get_quantity_strso that naming a unit typed as producing aQuantity. That was wrong:get_quantity("")returnsNone, and the empty string is exactly how dascore spells "carries no units". It would also have hidden a real error class, since an unset unit reaching arithmetic is what the optional return is meant to surface. Reverted; the docstrings now say so explicitly.A counterpart review by Codex is recorded in
.scratch/(untracked). It found theresamplebug, a false claim in thefilter_dfdocstring, and a test where a comprehension silently discarded results; all three are fixed here.Verified with ty 0.0.65 on Python 3.11, 3.12, 3.13 and 3.14 — the per-version sweep matters, since the pre-commit hook resolves a different interpreter than
--python .venvalone implies. Full suite 8135 passed / 251 skipped / 2 xfailed;pre-commit run --all-filesgreen.Changelog
get_filter_unitsopen bounds,filter_df'sSeriesreturn,get_coord's sequence orintdata,dc.read's(value, ...)ranges, anddc.write's path type.patch.resample(dim=None)raisesParameterErrornaming the dimension instead of failing later withValueError: cannot convert float NaN to integer.Checklist
I have (if applicable):