refactor: three small simplifications in the eager lane and piecewise - #366
Conversation
The tranche left over from the duplication review. - `_eval_node`'s three connectives each re-passed the same four arguments to recurse; a nested `evaluate` carries them, so NOT/AND/OR are one line each and what differs between them is the operator alone. - `"X does not support type '...'"` was written three times across `group_sum`, `shift(edge=wrap)` and `shift`. One `_unsupported` builds it, and its docstring can now say the thing all three needed to: this is reachable only from a hand-built call, since a lane running the language proper hands every helper an operand from `_eval_ast`. - `_validate_block` checked its emitted names against the schema in three loops — one for variables, one for the numbered links, one for the three fixed constraints — with the link message a near-copy of the constraint one. Now one list of emitted names per kind, so adding an emitted declaration is a name in a tuple rather than a fourth loop to remember. `lowering._check_dim_rules` was the fourth candidate and is deliberately untouched: it is a one-line wrapper over `dims_of`, but its *name* says what the bare call does not, and its docstring is the argument for why lowering asks `dimensions` rather than deciding again. Inlining it would either lose that or repeat it three times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
1 participant
The tranche left over from the duplication review. No behaviour change —
700 passing, ruff clean, pyrefly 0 errors.
_eval_node's connectives. NOT/AND/OR each re-passed the same fourarguments to recurse, twice per binary case. A nested
evaluatecarriesthem, so the three cases are one line each and what differs between them is
the operator alone.
"X does not support type '...'"× 3 —group_sum,shift(edge=wrap),shift. One_unsupportedbuilds it, and having one home lets itsdocstring state the thing all three sites needed and none of them said:
this is reachable only from a hand-built call, because a lane running the
language proper hands every helper an operand that came from
_eval_ast.piecewise._validate_blockchecked its emitted names in three loops —variables, the numbered links, then the three fixed constraints — with the
link message a near-copy of the constraint one. Now one list of emitted
names per kind, so a new emitted declaration is a name in a tuple rather
than a fourth loop someone has to remember to add.
Not done
lowering._check_dim_ruleswas the fourth candidate on the list and I left italone. It is a one-line wrapper over
dims_of(node, schema, context), butits name says what the bare call doesn't, and its docstring carries the
argument for why lowering asks
dimensionsfor the verdict instead ofdeciding a second time. Inlining it across the three call sites would either
drop that argument or copy it three times.
🤖 Generated with Claude Code