Enable ty rules: not-iterable, call-non-callable, unsupported-operator, invalid-assignment - #796
Conversation
Fix all diagnostics for not-iterable, call-non-callable, unsupported-operator, and invalid-assignment, and remove their ignore lines from [tool.ty.rules]. Highlights: - ArrayLike is now Annotated[np.ndarray, ...] instead of object, which also removes many diagnostics under the still-ignored rules. - is_array/is_pathlike gained TypeGuard/TypeIs so their checks narrow. - get_quantity/get_factor_and_unit use plain unions instead of the misused str_or_none TypeVar; invert_quantity now returns None instead of raising TypeError when given empty units. - get_array and BaseCoord.__getitem__ annotations no longer claim coord returns for array/scalar results. - TDMS unsupported-data-type error no longer raises TypeError for unknown type codes; spool.map guards os.cpu_count() returning None. - Refresh the burn-down counts comment in pyproject.toml.
- convert_units accepts numeric | Quantity as its tests document. - get_factor_and_unit accepts the datetime/timedelta inputs it supports. - invert_quantity signature reflects the Quantity it actually returns. - patches_to_df keeps accepting any object exposing get_contents(). - fbe uses 1 / sample_rate directly (get_dim_sampling_rate returns float).
📝 WalkthroughWalkthroughThe PR updates type annotations, adds runtime assertions, and refines input handling across core, I/O, processing, and utility modules. It also updates the ChangesCore data and coordinate handling
I/O resource and metadata handling
Processing and unit handling
Utility contracts and type-checker cleanup
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
dascore/io/index/planned.py (1)
128-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
intcast aroundround.Ruff reports RUF046 for this expression. Use
round((hi - lo) / step) + 1; it preserves the current behavior for the supported numeric and time branches.🤖 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 128 - 130, In the planned range length calculation, update the expression assigned to length by removing the redundant int cast around round while preserving the existing formula and supported numeric/time behavior.Source: Linters/SAST tools
dascore/io/sintela/utils.py (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse strict zips for fixed-length metadata pairs.
Ruff reports B905 at all three calls. Add
strict=Truewhen the supported Python target is 3.10 or newer. Otherwise, configure the lint rule for the declared target.
dascore/io/sintela/utils.py#L106-L108: addstrict=Truetozip(names, array[0]).dascore/io/sintela/utils.py#L135-L137: addstrict=Truetozip(names, buf[0]).dascore/io/tdms/utils.py#L180-L180: addstrict=Truetozip(FILEINFO_NAMES, fields).🤖 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/sintela/utils.py` around lines 106 - 108, Update the zip calls at dascore/io/sintela/utils.py lines 106-108 and 135-137, and dascore/io/tdms/utils.py line 180, to pass strict=True for these fixed-length metadata pairs; if the declared Python target is below 3.10, configure Ruff’s B905 rule instead.Source: Linters/SAST tools
🤖 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/utils/models.py`:
- Around line 39-41: Update the ArrayLike annotation and its validator typing to
use a structural protocol representing the duck-typed contract accepted by
compat.array(), rather than np.ndarray. Ensure BaseCoord.data and related
coordinate paths retain foreign array-like values while exposing only operations
guaranteed by that protocol, and keep array() behavior unchanged.
In `@dascore/utils/progress.py`:
- Line 41: Update the progress handling around the `sequence` length calculation
so unsized iterables do not reach the `length < min_length` comparison with
`length` set to None. Normalize an unknown length to zero before that
comparison, or otherwise require an explicit length consistent with the existing
API.
---
Nitpick comments:
In `@dascore/io/index/planned.py`:
- Around line 128-130: In the planned range length calculation, update the
expression assigned to length by removing the redundant int cast around round
while preserving the existing formula and supported numeric/time behavior.
In `@dascore/io/sintela/utils.py`:
- Around line 106-108: Update the zip calls at dascore/io/sintela/utils.py lines
106-108 and 135-137, and dascore/io/tdms/utils.py line 180, to pass strict=True
for these fixed-length metadata pairs; if the declared Python target is below
3.10, configure Ruff’s B905 rule instead.
🪄 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: 8037bcec-eecb-412e-b88c-f4a8b308b3c5
📒 Files selected for processing (30)
dascore/compat.pydascore/core/attrs.pydascore/core/coordmanager.pydascore/core/coords.pydascore/core/spool.pydascore/io/index/backend.pydascore/io/index/indexer.pydascore/io/index/planned.pydascore/io/segy/core.pydascore/io/sintela/utils.pydascore/io/tdms/utils.pydascore/io/wav/core.pydascore/proc/coords.pydascore/proc/filter.pydascore/proc/mute.pydascore/transform/fbe.pydascore/transform/fourier.pydascore/units.pydascore/utils/chunk_plan.pydascore/utils/jit.pydascore/utils/mapping.pydascore/utils/misc.pydascore/utils/models.pydascore/utils/moving.pydascore/utils/patch.pydascore/utils/patch_assembly.pydascore/utils/paths.pydascore/utils/pd.pydascore/utils/progress.pypyproject.toml
| ArrayLike = Annotated[ | ||
| object, | ||
| np.ndarray, | ||
| PlainValidator(array), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep ArrayLike compatible with preserved array-like values.
Line [40] declares ArrayLike as np.ndarray, but dascore.compat.array() preserves foreign array-like objects accepted by is_array_like(); see dascore/compat.py, lines [53-95]. BaseCoord.data and related coordinate paths can therefore contain values that are not np.ndarray.
This annotation can make valid duck-typed inputs fail static checks. It can also let ty assume NumPy-only operations without a required conversion.
Use a structural protocol matching the supported array-like contract. If foreign array-likes are no longer supported, change array() and its documentation instead.
Suggested direction
ArrayLike = Annotated[
- np.ndarray,
+ ArrayLikeProtocol,
PlainValidator(array),
]🤖 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/models.py` around lines 39 - 41, Update the ArrayLike
annotation and its validator typing to use a structural protocol representing
the duck-typed contract accepted by compat.array(), rather than np.ndarray.
Ensure BaseCoord.data and related coordinate paths retain foreign array-like
values while exposing only operations guaranteed by that protocol, and keep
array() behavior unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #796 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17700 17717 +17
=========================================
+ Hits 17700 17717 +17
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:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f131af65bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ArrayLike = Annotated[ | ||
| object, | ||
| np.ndarray, |
There was a problem hiding this comment.
When a patch is constructed with a non-NumPy object exposing shape, dtype, and an array protocol (for example a lazy or Array API object), PlainValidator(array) deliberately returns that object unchanged because compat.array() preserves array-likes. Declaring the result as np.ndarray therefore makes Patch.data, coordinates, and get_array() statically promise ndarray-only members that may not exist at runtime, allowing the newly enabled checker to approve code that then fails for supported array-like inputs. Use a protocol or union that reflects the objects the validator actually preserves.
Useful? React with 👍 / 👎.
|
|
||
| def track( | ||
| sequence: Sized | Generator, | ||
| sequence: Iterable, |
There was a problem hiding this comment.
For an unsized iterable newly admitted by this annotation, such as iter([1, 2]) or map(...), and with length omitted, len(sequence) raises inside the suppressed block and leaves length as None; the subsequent length < min_length then raises TypeError before anything is yielded. Either default length to zero when length detection fails or keep the parameter type restricted to inputs the implementation can handle.
Useful? React with 👍 / 👎.
| @abc.abstractmethod | ||
| def __getitem__(self, item) -> Self: | ||
| """Should implement slicing and return new instance.""" | ||
| def __getitem__(self, item): |
There was a problem hiding this comment.
Replacing the inaccurate Self annotation by deleting the return type makes calls through the public BaseCoord interface resolve to an unknown/untyped result, so the newly enabled checker cannot validate downstream scalar-versus-coordinate usage at all. The method's documented behavior already identifies the required distinction, so annotate it with an appropriate coordinate-or-scalar union rather than removing the hint. .agents/agents.mdL75-L80
Useful? React with 👍 / 👎.
- track() no longer raises TypeError for unsized iterables passed without an explicit length; it now just skips the progress bar (with test). - BaseCoord.__getitem__ regains typed signatures via overloads: int indices yield a value, slice/array indices a coord. - Comment on ArrayLike records why ndarray is its static face even though the validator can preserve duck array-likes.
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/core/coords.py (1)
1148-1158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRoute every invalid reducer to
ParameterError.
_AGG_FUNCS.get(dim_reduce)requires a hashable key. A list or dictionary raisesTypeError. A NumPy array can fail earlier duringdim_reduce == "empty"with an ambiguous-truth-valueValueError. These inputs bypass the existingParameterErrorpath. Check named reducers only for string values, then route all other invalid non-callables toParameterError.Proposed fix
- if dim_reduce == "empty": + if isinstance(dim_reduce, str) and dim_reduce == "empty": if len(self) == 1: return self new_coord = get_coord(shape=(1,), units=self.units, dtype=self.dtype) - elif dim_reduce == "squeeze": + elif isinstance(dim_reduce, str) and dim_reduce == "squeeze": return None else: - func = dim_reduce if callable(dim_reduce) else _AGG_FUNCS.get(dim_reduce) + func = ( + dim_reduce + if callable(dim_reduce) + else _AGG_FUNCS.get(dim_reduce) + if isinstance(dim_reduce, str) + else None + )🤖 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/coords.py` around lines 1148 - 1158, Update the reducer selection around dim_reduce, dim_reduce == "empty", and _AGG_FUNCS.get so named reducer checks occur only for string values; preserve callable reducers, and route every other non-callable value—including unhashable or array-like inputs—to ParameterError without allowing TypeError or ambiguous-truth-value errors to escape.
🤖 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/core/coords.py`:
- Around line 1148-1158: Update the reducer selection around dim_reduce,
dim_reduce == "empty", and _AGG_FUNCS.get so named reducer checks occur only for
string values; preserve callable reducers, and route every other non-callable
value—including unhashable or array-like inputs—to ParameterError without
allowing TypeError or ambiguous-truth-value errors to escape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0531d74-1eb3-448f-98ce-54f3d42adcd9
📒 Files selected for processing (4)
dascore/core/coords.pydascore/utils/models.pydascore/utils/progress.pytests/test_utils/test_progress.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dascore/utils/models.py
- dascore/utils/progress.py
1 participant
Description
First burn-down PR following #795: re-enables the four smallest ignored ty rules —
not-iterable,call-non-callable,unsupported-operator, andinvalid-assignment— fixes all of their diagnostics, and deletes theirignorelines from[tool.ty.rules]so new violations fail pre-commit/CI from now on.The fixes make the hints tell the truth rather than contorting code:
ArrayLike(utils/models.py) is nowAnnotated[np.ndarray, ...]instead ofAnnotated[object, ...]— the validator already guarantees an ndarray. This one change also removed a large chunk of diagnostics under the still-ignored rules (no-matching-overload41→12,not-subscriptable36→19).is_arrayandis_pathlikeare nowTypeGuard/TypeIspredicates, so existing guard branches narrow properly.get_quantity/get_factor_and_unituse plain unions instead of the misusedstr_or_noneTypeVar;get_arrayno longer claims to return aBaseCoord; the abstractBaseCoord.__getitem__no longer claimsSelffor int indices;trackacceptsIterable;yield_range_tuple_from_kwargsis annotated as the generator it is.assert x is not Noneguards document invariants ty can't derive (loop-set variables, structured-dtype field names, registry lookups callers pre-validate).# ty: ignorecomments remain, both for checker limitations rather than code problems (a union-of-time-kinds arithmetic inio/index/planned.pyand a TypeVar-intersection artifact inunits.py); both are commented and greppable.Small behavior fixes the rules flushed out:
invert_quantity("")now returnsNoneinstead of raisingTypeError.TypeErrorwhen the type code is unknown.spool.mapno longer divides byNonewhenos.cpu_count()returnsNone.The ignored-rule counts comment in
pyproject.tomlis refreshed (invalid-argument-type 179, unresolved-attribute 138, invalid-return-type 72, invalid-method-override 35, not-subscriptable 19, no-matching-overload 12).Changelog
none
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Refactor