Enable ty's unresolved-attribute rule - #797
Merged
Merged
Conversation
Take unresolved-attribute from 138 diagnostics to 26 and enforce the rule everywhere except core/coords.py and core/spool.py, which are held back by a documented temporary override until the coord/spool API questions behind them are settled. Most fixes are hints that were wrong: optional_import is overloaded so its default raise-on-missing mode returns a module; the h5 caster classes inherit the managed handle under TYPE_CHECKING (FiberIO methods annotate the caster but receive the handle); the index reads its frames by column instead of through per-row namedtuples; the xml_binary helpers take XMLBinaryInfo, not XMLLaserZones; to_summary_dict only ever returns CoordSummary. The dynamic markers stamped by the type-caster and patch_function decorators are now declared as Protocols. Three latent bugs surfaced along the way: a NaT timedelta bound crashed _coord_record_from_row, maybe_mem_map leaned on AttributeError from fid.name, and the binary-ufunc error path raised AttributeError instead of UnitError for unitless operands.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refines static typing across coordinate, I/O, utility, and index modules. It adds typed index row views, strict column-wise processing, and runtime state checks. ChangesTyping and index processing
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #797 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 164 164
Lines 17717 17824 +107
==========================================
+ Hits 17717 17824 +107
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:
|
![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: 4
🤖 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/schema.py`:
- Line 281: Update the type annotations for PatchRecord.sample_count_total and
CoordRecord.length from int to an optional integer type. Ensure both row-view
fields explicitly allow None before backend insertion, matching the NULL values
stored in rows.
In `@dascore/units.py`:
- Around line 385-386: Replace the assertion guarding to_quant in the public
to_unit handling with explicit validation: reject None or quantities whose
magnitude is not 1.0 by raising UnitError before accessing to_quant.units.
Preserve the existing valid-unit path and assign to_units only after validation.
In `@dascore/utils/patch.py`:
- Around line 97-99: Update the method_name history-formatting branch to use the
same unnamed-callable fallback as the full history string, reusing the existing
callable-name resolution instead of accessing __name__ directly.
In `@dascore/utils/pd.py`:
- Around line 25-38: Update iter_rows to call DataFrame.itertuples with the
index excluded, so each typed row contains only dataframe column values matching
row_type; preserve the existing iterator cast and public function behavior.
🪄 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: b3f3ce83-ccd8-474c-aa14-20209bc7581e
📒 Files selected for processing (23)
dascore/core/coordmanager.pydascore/core/coords.pydascore/io/core.pydascore/io/index/backend.pydascore/io/index/catalog.pydascore/io/index/indexer.pydascore/io/index/ingest.pydascore/io/index/planned.pydascore/io/index/query.pydascore/io/index/schema.pydascore/io/tdms/utils.pydascore/io/xml_binary/utils.pydascore/proc/coords.pydascore/units.pydascore/utils/array.pydascore/utils/hdf5.pydascore/utils/jit.pydascore/utils/misc.pydascore/utils/patch.pydascore/utils/patch_assembly.pydascore/utils/pd.pypyproject.tomltests/test_io/test_index/test_schema.py
The memmap name lookup moves back inside the try so a file object whose name property raises still falls back to the in-memory read, iter_rows passes index=False so the declared row shape lines up positionally with what pandas yields, and a stack shallower than the requested level now returns "<unknown>" instead of asserting (the helper runs while building error messages, where crashing would mask the real error).
Mark the two stored columns records can leave NULL (patches sample_count_total, coord_defs length) optional in the row views, share one name helper between the full and method_name history strings so unnamed callables work in both, and raise UnitError for a to_unit that is not a magnitude-1 unit instead of asserting on it.
The table column dicts and the row views said the same thing twice, kept in step by a test. Declare each table as a NamedTuple naming its columns in order and derive TABLES (the logical types the DDL is built from) from the annotations, so the row a reader sees and the columns SQLite gets cannot disagree. The per-column commentary moves onto the fields, and nullability is now recorded (as `| None`) where it previously was not. The derived mapping is identical to the literals it replaces, so the emitted DDL is unchanged. The drift tests give way to one end-to-end check that a created index's columns and types match the declaration.
Answer the two API questions the rule raised instead of holding the files back with a per-file override. A PatchCatalog without a resolver holds rows it can never resolve, and all six construction paths already passed one, so resolver becomes a required keyword argument and the None branches it justified go away. Spool._catalog is declared as a PatchCatalog rather than defaulting to None; __init__ sets it on every path (copy-construction and unpickling included), so its None guards go with it. BaseCoord promises _get_index, which now has a base implementation: get_next_index on a sorted string coord raised AttributeError, and string coords have no value spacing to index into, so it raises CoordError like the other unsupported string-coord operations. start and stop stay CoordRange's, so approx_equal narrows with isinstance. Also fix the sorted/evenly_sampled/reverse_sorted return annotations (they return bools, not tuples) and cover the NaT timedelta envelope.
The per-table alias constants were a third name for what the row classes and TABLES already say; three of the seven had no users left. Inserts now take their column list from the row class whose tuples they are inserting.
d-chambers
changed the title
String coords do have lexicographic insertion positions; what they lack is positional semantics, by policy. And iter_rows names the row shape for readers and type checkers only, not at runtime.
4 tasks
This was referenced Aug 5, 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
Second step of the ty burn-down started in #795/#796.
unresolved-attributegoes from 138 diagnostics to zero and the rule is enabled everywhere — no per-file holdbacks.The rule earns its keep: it found real latent bugs, and most of the other fixes are hints that were simply wrong.
Root-cause fixes (each removes a cluster)
optional_importis now overloaded: with the defaulton_missing="raise"it returns a module, notModuleType | None. That alone silences ~15 false "attribute not defined on None" reports at the segyio/obspy/xarray/findiff call sites.H5Reader/H5Writerinherit_ManagedH5pyFileunderTYPE_CHECKING. FiberIO methods annotate the caster class, but the io machinery hands them the managed handleget_handlereturns, so the annotation now describes the value the method actually receives. Runtime is untouched.itertuples()loops read their columns directly (zip(meta["attr_name"], …, strict=True)) instead of building a namedtuple per row — faster, and no dynamic row type is left to resolve. The one loop that genuinely needs attribute-style rows (assemble_source_records, which doesgetattr(row, field)over record fields) keepsitertuplesbehinditer_rows(df, Row). Those row classes are now the index schema itself: each stored table is declared once as aNamedTuple, andTABLES(the logical column types the DDL is built from) is derived from the annotations, so what a reader sees and what SQLite gets cannot disagree. The derived mapping is identical to the literals it replaces, so the emitted DDL is unchanged and noINDEX_VERSIONbump is needed; nullability is now recorded (| None) though not yet enforced asNOT NULL.io/xml_binary/utils.pyhelpers were annotatedXMLLaserZonesbut takeXMLBinaryInfo, andCoordManager.to_summary_dictclaimed it could returntuple[str, ...]when it only ever returnsCoordSummary._TypeCasterMethod(io/core) and_PatchFunction(utils/patch) are Protocols naming the markers those decorators stamp onto the functions they wrap.The two API questions the rule raised, answered
PatchCatalogexist without a resolver? No — a catalog without one holds rows it can never resolve, and every construction path (from_patches,from_file,from_directory,union,_view, and the two direct calls) already passed one.resolveris now a required keyword argument, which makes the None branches in__getstate__,resolve_row,remove, andSpool.has_live_patchesdead code; they are gone.BaseCoordpromise?values, which every coord exposes but the array-backed coords store as a pydantic field while the rest compute in a property (pydantic refuses to let a field shadow an inherited property, and a field would makevaluesa required init argument, so it is declared for type checkers only); and_get_index, which now has a base implementation — see below.start/stopare not promised:approx_equalreaches for them only for evenly sampled coords, which are exactlyCoordRange, so it narrows withisinstanceinstead.Requiring
resolverdoes break a barePatchCatalog()/PatchCatalog(backend=...), which used to construct and then fail at resolution time. That constructor has never been in a release (the index landed in #751, after v0.1.20) and every documented path uses the factories, so this seemed the moment to make it say what it needs. It stays keyword-only, so nothing positional moves.Spool._catalogis likewise declared as aPatchCatalograther than defaulting toNone.__init__sets it on every construction path (including copy-construction and unpickling), so the None guards inindexer,has_live_patches, andupdatewent with it.Latent bugs fixed
get_next_indexon a sorted string coord raisedAttributeError: 'CoordString' object has no attribute '_get_index'. String coords have no value spacing to index into, soBaseCoord._get_indexnow raisesCoordErrorthe way the other unsupported string-coord operations do._coord_record_from_rowcrashed on a NaT timedelta bound (pd.NaT.to_timedelta64does not exist); it now usesdc.to_timedelta64, which handles NaT._apply_binary_ufunc's dimensionality-error handler readother.unitswhile building the message, which the type is not required to carry.Other
CoordManager._get_dim_array_dict(keep_coord=...)is split into_get_dim_coord_dict()and_get_dim_array_dict(); the boolean was a return-type switch with three call sites.BaseCoord.sorted,.evenly_sampled, and.reverse_sortedwere annotated-> tuple[int, ...]; they return bools.PatchResolver.live_entries()is typeddictrather thanMapping: every implementation returns the registry dict andremove()pops from it.maybe_mem_mapasks for the name it needs (getattr(fid, "name", None)) instead of relying onAttributeErrorfromfid.name.assertwith a comment stating the invariant, plus two tagged inline ignores (pydantic hidesBaseModel.__getattr__from checkers on purpose; the numba module object is the dummy shim in the not-installed branch).The counts comment in
pyproject.tomlis refreshed:invalid-argument-type171,invalid-return-type69,invalid-method-override36,no-matching-overload13,not-subscriptable5. Bringingtests/into scope (133 diagnostics under the rules already enabled) stays for a later step.Changelog
none
Checklist
I have (if applicable):