Enable ty's invalid-return-type rule - #840
Merged
Merged
Conversation
_yield_attrs_coords is a generator yielding three-tuples, not a two-tuple; both callers already unpack three. The coordmanager drop helpers delegate to drop_coords, which returns (cm, array), and the tests already index [0]. get_xarray_data_var_name deliberately returns None for XDAS payloads. _is_dasvader_jld2 handed back the empty set instead of False when no data name matched, which is the one runtime change here.
pass_filter in the same module already does this. dc.Patch(...) drops any subclass on the floor while the signature promises PatchType; proc.update constructs through self.__class__, so the promise holds. Verified equal for both shapes: data-only and data plus coords/attrs.
CoordManager.update and CoordPartial.change_length both build through a factory whose declared return is wider than the case at hand.
unit_str is None for a coord with no units, size is a builtin int only if math rather than numpy computes it, and get_next_index hands back an array for a sized value and a numpy integer otherwise.
A quantity of None survives assert_dtype_compatible_with_units for a non-time dtype. quant_sequence_to_quant_array really does return a Quantity; numpy just declares ndarray.__mul__ as returning an ndarray, so pint's reflected operator is invisible.
One dict held both frozensets and tuples under a discriminating key prefix; two dicts keep each to a single value type and drop the tuple allocation per lookup. write returns the path it was handed, which is whatever path_types allows, not necessarily a Path.
scan_payload already returned _make_scan_payload results, so naming ScanPayload fixes both it and the sintela FiberIO. The unit filter now skips None instead of differencing it out, which narrows. The deprecate wrapper drops the repo's only (and unused, since no mypy runs here) type: ignore in favor of a cast.
The last ignored ty rule is now on. The overload implementations that ty cannot verify are documented as such rather than annotated to look checked.
Contributor
📝 WalkthroughWalkthroughThe PR corrects public and internal type annotations, updates coordinate manager return contracts, preserves patch subclasses through processing operations, separates FiberIO caches, fixes DASvader boolean detection, and documents the API changes. ChangesTyping and API corrections
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
![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: 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/io/core.py`:
- Around line 431-434: Update the key type annotations for _input_type_cache and
_prioritized_cache to str | None, preserving their existing value types and
cache behavior so None keys accepted by yield_fiberio and its helpers are
represented correctly.
🪄 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: a66de8b6-7be4-4d02-9ab7-5d9638e145ee
📒 Files selected for processing (17)
dascore/core/coordmanager.pydascore/core/coords.pydascore/io/core.pydascore/io/dasvader/utils.pydascore/io/febus/a1utils.pydascore/io/index/query.pydascore/io/netcdf/utils.pydascore/io/prodml/core.pydascore/io/sintela/protobuf_utils.pydascore/proc/filter.pydascore/transform/strain.pydascore/units.pydascore/utils/deprecate.pydascore/utils/misc.pydocs/changelog.qmdpyproject.tomltests/test_io/test_dasvader/test_dasvader.py
Comment on lines
+431
to
+434
| # Kept as two dicts rather than one keyed by a discriminating | ||
| # prefix so each stays a single value type. | ||
| self._input_type_cache: dict[str, frozenset[FiberIO]] = {} | ||
| self._prioritized_cache: dict[str, tuple[FiberIO, ...]] = {} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include None in both cache key types.
yield_fiberio accepts input_type: str | None and can pass None to both cache-backed helpers. Both caches can therefore contain a None key, but their declarations allow only str. Change both key types to str | None.
Proposed type correction
- self._input_type_cache: dict[str, frozenset[FiberIO]] = {}
- self._prioritized_cache: dict[str, tuple[FiberIO, ...]] = {}
+ self._input_type_cache: dict[str | None, frozenset[FiberIO]] = {}
+ self._prioritized_cache: dict[str | None, tuple[FiberIO, ...]] = {}📝 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.
Suggested change
| # Kept as two dicts rather than one keyed by a discriminating | |
| # prefix so each stays a single value type. | |
| self._input_type_cache: dict[str, frozenset[FiberIO]] = {} | |
| self._prioritized_cache: dict[str, tuple[FiberIO, ...]] = {} | |
| # Kept as two dicts rather than one keyed by a discriminating | |
| # prefix so each stays a single value type. | |
| self._input_type_cache: dict[str | None, frozenset[FiberIO]] = {} | |
| self._prioritized_cache: dict[str | None, tuple[FiberIO, ...]] = {} |
🤖 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/core.py` around lines 431 - 434, Update the key type annotations
for _input_type_cache and _prioritized_cache to str | None, preserving their
existing value types and cache behavior so None keys accepted by yield_fiberio
and its helpers are represented correctly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #840 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17945 17948 +3
=========================================
+ Hits 17945 17948 +3
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: |
d-chambers
added a commit
that referenced
this pull request
Aug 8, 2026
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.
This was referenced Aug 8, 2026
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
Takes ty's
invalid-return-typefrom 26 diagnostics to 0 and enables the rule, following #830 and #836.[tool.ty.rules]is now empty — no rule is ignored any more.No suppressions were added, and one was removed. The
# type: ignore[return-value]indascore/utils/deprecate.pywas the only one in the codebase and did nothing, since no mypy runs here; acastreplaces it.Six of the 26 turned out to be annotations that lied about runtime behaviour:
_yield_attrs_coordsdeclaredtuple[dict, CoordManager], but it is a generator yielding three-tuples — both callers already unpack three.CoordManager.drop_disassociated_coordsanddrop_private_coordsdeclaredSelf;drop_coordsreturns(cm, array), and the tests already index[0].get_xarray_data_var_namedeliberately returnsNonefor XDAS payloads._is_dasvader_jld2returned the empty set rather thanFalsewhen no data name matched. The one runtime fix, with a test that fails without it.BaseCoord.sizereturned annp.int64(and1.0for a shapeless coord); it now usesmath.prod.BaseCoord.get_next_indexdeclaredintwhile returning an array for sized input.In three places the type error was pointing at a design smell rather than a missing annotation:
sobel_filter,notch_filterandvelocity_to_strain_rate_edgelessbuilt their result with a baredc.Patch(...), dropping any subclass while promisingPatchType.pass_filter, in the same module, already usedpatch.new(...). Equality was verified at runtime for both call shapes.scan_payloadalready returned_make_scan_payloadresults, so namingScanPayloadfixed both it and the SintelaFiberIO._lookup_cacheheld frozensets and tuples under a discriminating key prefix. Two typed dicts drop the union and a tuple allocation per lookup.Three casts remain, each documented at the site:
CoordManager.updateandCoordPartial.change_lengthbuild through factories whose declared return is wider than the case at hand, and thedeprecatewrapper stands in for the function it wraps in a way(*args, **kwargs)cannot express.The two remaining
@overloadsets in the repo (BaseCoord.__getitem__,unbyte) are not verifiable by ty at any annotation: a falsification probe with a deliberately wrong overload produces zero diagnostics, because their honest implementation returns containAnyor an unsolved TypeVar, which absorbs anything. They are left unannotated with a comment saying so, rather than annotated to look checked.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.Next: widen
[tool.ty.src]to covertests/.Changelog
BaseCoord.sizeis a builtinintrather than annp.int64, and1rather than1.0for a shapeless coord.Patch.sobel_filter,Patch.notch_filter, andvelocity_to_strain_rate_edgelessbuild their result withpatch.new(...), so aPatchsubclass survives them as their signatures promised.Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
dc.scannow returnsPatchSummaryobjects.