Fix the last public signatures that misdescribed themselves - #845
Conversation
ExecutorType declared a named iterables parameter and a **kwargs catch all; ThreadPoolExecutor and ProcessPoolExecutor provide neither, so the documented way to parallelize a spool.map was a type error. CoordSummary required dtype even though its before-validator derives it from min. Both were found by running ty over tests/, which is deliberately not in scope -- the scan audits whether the public signatures are honest, and pyproject now records that and how to repeat it.
Defaulting dtype meant a mapping the validator skipped would quietly get an empty one instead of raising, so the validator now handles any Mapping rather than only a dict.
Its docstring says non-array inputs return False and the body guards on hasattr(data, 'dtype'), so the parameter is not ArrayLike.
Spelling the second parameter as *iterables demanded arbitrarily many iterables, excluding single-iterable clients and anything shaped like the protocol it replaced; DASCore calls map with exactly two positional arguments, so that is what it now declares. Annotating them also stops a map of the wrong shape from satisfying the protocol and failing at runtime, which was true before this branch too. Defaulting the coord summary dtype left attribute-based validation with an empty one, which the indexer treats as an unsupported coordinate, so it is now derived after validation as well.
|
Warning Review limit reached
Next review available in: 31 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 (6)
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 #845 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 164 164
Lines 17953 17961 +8
=========================================
+ Hits 17953 17961 +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:
|
![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: cac0992d71
ℹ️ 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".
| if not self.dtype: | ||
| dtype = _get_dtype(self.min, None) | ||
| object.__setattr__(self, "dtype", str(dtype).split("[")[0]) |
There was a problem hiding this comment.
When model_validate(..., from_attributes=True) receives an object with a numeric min but castable string values such as max="2.0" and step="0.5", this validator derives float64 but leaves max and step as strings because only the mapping path calls ensure_consistent_dtype. The resulting summary fails in to_coord() at np.sign(step), whereas the equivalent mapping input is normalized successfully; normalize min, max, and step on the attribute-based path as well.
Useful? React with 👍 / 👎.
Attribute-based validation derived a dtype from min but left max and step untouched, so a summary could carry float64 alongside string values and fail in to_coord's np.sign. Conform the values in the same branch that derives the dtype, which keeps this path in agreement with the mapping one at no cost to the mapping path itself.
1 participant
Description
Closes out the
tyadoption line.dascore/has been fully checked with every rule enabled since #840; this fixes the last public-API signatures that misdescribed themselves, and records whytests/stays out of scope.How these were found
Running
tyovertests/is an audit of whether dascore's public signatures are honest, because tests call the API the way users do.tests/is not added to[tool.ty.src]here and is not intended to be — the value is in the scan, not in enforcement.pyproject.tomlnow records that reasoning and the one-line command to repeat it, so the question does not have to be re-litigated:uvx ty@0.0.65 check --python .venv --config "src.include=['dascore','tests']"Fixed
spool.map(..., client=ThreadPoolExecutor())never type checked.ExecutorTypedeclared a namediterablesparameter and a**kwargscatch-all; neitherThreadPoolExecutornorProcessPoolExecutorprovides either, so the executors the protocol exists to accept structurally failed it. It now declares the two positional parameters DASCore actually calls it with, so single-iterable clients and anything matching the old shape still satisfy it. The parameters are annotated as well, which stops amapof the wrong shape (one taking anint) from satisfying the protocol and then failing at runtime withTypeError: 'int' object is not iterable— a hole that predates this branch. Runtimeisinstanceis unaffected;runtime_checkableignores signatures.CoordSummaryrequireddtypeeven though it is derived fromminduring validation. Now defaulted and derived on every validation path, including non-dict mappings andfrom_attributes=True._is_text_coercible_arraywas annotatedArrayLikewhile its own docstring promises non-array inputs returnFalse, which is exactly what itshasattr(data, "dtype")guard implements.Deliberately not changed
BinaryReaderand friends inFiberIOsignatures are markers the resource-casting machinery reads at runtime throughget_type_hints, so they cannot be widened to describe what callers may pass without breaking the mechanism.get_quantity(...), which is correct to flag: the empty string is how dascore spells "carries no units", so an unset unit reaching a multiplication is a real error. The typed path already exists —dc.units.mandfrom dascore.units import mboth type check in arithmetic.A counterpart review by Codex is recorded in
.scratch/(untracked). It found that the first version of the executor protocol excluded single-iterable clients, that defaulting the dtype opened afrom_attributeshole where the indexer would then skip the coordinate, and that the changelog overclaimed on both; all are fixed here. Two further defects were caught by probing my own claims before it returned.Verified with ty 0.0.65 on Python 3.11, 3.12, 3.13 and 3.14. Full suite 8138 passed / 251 skipped / 2 xfailed;
pre-commit run --all-filesgreen.Changelog
spool.map(..., client=...)accepts the standard library executors; theExecutorTypeprotocol declared parameters neitherThreadPoolExecutornorProcessPoolExecutorprovides, so passing one was a type error even though it always worked.CoordSummaryno longer requiresdtype— it is derived frommin, andmin/max/stepare conformed to the derived type so they cannot contradict it.Checklist
I have (if applicable):