refactor: one near-miss clause, one fold, and comments that restate their line - #361
Merged
Merged
Conversation
…heir line What survived rebasing onto #359 — the engine-side half of this pass (the label arithmetic, `plan.children`, `dataclasses.replace` over `TermFragment`, the shared shift-edge filter) landed with that branch, and the duplicated `_check_where` call it also fixed is upstream too. DRY: - The "Did you mean …?" clause was written three times — for an unknown declaration name, an unknown YAML key, and an unknown symbol-table entry. `errors.did_you_mean` is now the clause and each caller keeps its own framing sentence, which is the part that genuinely differs. `errors.py`'s own docstring had already noted there were two copies before it added a third. Drops the `difflib` import from `schema.py` and `typeset/symbols.py`. - `_fold_and` and `_fold_or` differed only in the node class; `_folder` takes it as an argument. The fold is the grammar's associativity, which is one rule. - `_resolve_where` tested `isinstance(value, str) and ns.kind(value) == ...` three times in sequence for the three things a where-comparison's RHS may not name. One walrus and a `_declared_rhs_error` dispatch; the three messages stay distinct, because only the dimension case is a silent wrong answer rather than an obvious one. - `Namespace._unknown` built its two kind labels and their two value lists through parallel ternaries that had to stay in step; one list of pairs. Comments: removed the ones that restate the line below them — the per-`isinstance` branch labels in `_coerce_to_dataarray`, `# Step 2:` and `# Step 5:` numbering a sequence that no longer exists, `# Atoms` / `# Unary` / `# Function calls` over the grammar they name, `# Referenced dimensions must be declared`. Every comment carrying an argument stays, and where a section label was doing real work it was rewritten into one instead (`# at most one comparison, and only at the top`, `# NOT binds tightest, then AND, then OR`). Also drops `expression_parser.RESOLVED_REFERENCE_NODES`, unused, and whose comment claimed both consumers asserted on it when neither did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change centralizes unknown-name suggestions, simplifies namespace diagnostics, consolidates where-expression folding, reorganizes grammar definitions, and removes obsolete internal comments. Runtime parsing, validation, coercion, and error behavior remain unchanged except for shared diagnostic construction. ChangesDiagnostics and parser maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 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 `@src/lpspec/resolution.py`:
- Around line 361-386: The diagnostics from _declared_rhs_error must not infer
the left-hand operand’s kind from the RHS kind. Update the parameter, variable,
and dimension messages to describe only the invalid RHS reference, or extend the
helper and its _resolve_where callers with the actual left-hand kind; ensure
both parameter-to-dimension and dimension-to-parameter comparisons receive
accurate, neutral wording.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ec7420a-43d1-45a0-8fab-8427976cc673
📒 Files selected for processing (8)
src/lpspec/errors.pysrc/lpspec/expression_parser.pysrc/lpspec/linopy/builder.pysrc/lpspec/linopy/loader.pysrc/lpspec/resolution.pysrc/lpspec/schema.pysrc/lpspec/typeset/symbols.pysrc/lpspec/where_parser.py
💤 Files with no reviewable changes (2)
- src/lpspec/linopy/builder.py
- src/lpspec/linopy/loader.py
Comment on lines
+361
to
+386
| def _declared_rhs_error(context: str, node: UnresolvedComparisonNode, value: str, kind: str) -> str: | ||
| """Why the right-hand side of a where-comparison may not name a declaration. | ||
|
|
||
| One refusal — the RHS is a literal — but three distinct reasons, and the | ||
| wording has to say which, since only the dimension case is a *silent* wrong | ||
| answer rather than an obvious one. | ||
| """ | ||
| shown = f"'{node.name} {node.op} {value}'" | ||
| if kind == 'parameter': | ||
| return ( | ||
| f'{context}: {shown} compares two parameters, which is not in the ' | ||
| f'language — a where-comparison tests one parameter or dimension against ' | ||
| f'a literal. Precompute the comparison as a boolean parameter in data ' | ||
| f'prep and test that.' | ||
| ) | ||
| if kind == 'variable': | ||
| return f'{context}: {shown} compares against variable {value!r}. A where mask is built before variables exist.' | ||
| return ( | ||
| f'{context}: {shown} compares against dimension {value!r}, which the RHS reads ' | ||
| f'as the literal coordinate {value!r} — so the predicate tests one dimension ' | ||
| f"against another dimension's *name* and masks everything out. Comparing two " | ||
| f'dimensions to each other is not in the language; if {value!r} is a coordinate ' | ||
| f'rather than the dimension, rename one of the two.' | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the RHS diagnostic neutral about the left-hand kind.
_declared_rhs_error receives only the RHS kind, but _resolve_where accepts both parameters and dimensions as the left-hand name. Therefore, where: dimension > parameter is reported as “compares two parameters”, while where: parameter > dimension is described as a dimension-to-dimension comparison. Use wording based only on the RHS, or pass the left-hand kind into this helper.
Also applies to: 424-425
🤖 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 `@src/lpspec/resolution.py` around lines 361 - 386, The diagnostics from
_declared_rhs_error must not infer the left-hand operand’s kind from the RHS
kind. Update the parameter, variable, and dimension messages to describe only
the invalid RHS reference, or extend the helper and its _resolve_where callers
with the actual left-hand kind; ensure both parameter-to-dimension and
dimension-to-parameter comparisons receive accurate, neutral wording.
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.
A review pass over the package for duplication and for comments that restate
the code they sit above. No behaviour change — 700 passed, ruff clean,
pyrefly 0 errors.
This is what survived rebasing onto #359. The engine-side half of the same
pass (the row-major label arithmetic written once,
plan.children,dataclasses.replaceoverTermFragment, the shared acyclic-shift edgefilter) reached
mainwith that branch, as did the doubled_check_wherecall it fixed on the way. Only prose conflicted, and I kept
main's wording.DRY
errors.did_you_mean. The "Did you mean …?" clause was spelled threetimes — an unknown declaration name, an unknown YAML key, an unknown
symbol-table entry. Each caller keeps its own framing sentence, since that is
the part that actually differs; only the clause is shared.
errors.py'sdocstring had already observed there were two copies before it added the
third. Drops
difflibfromschema.pyandtypeset/symbols.py.where_parser._folder._fold_andand_fold_ordiffered only in thenode class. The fold is the grammar's associativity, which is one rule.
resolution._declared_rhs_error._resolve_whereranisinstance(value, str) and ns.kind(value) == ...three times in sequence forthe three things a where-comparison's right-hand side may not name — six lines
above a
match ns.kind(...)doing the same job. Now one walrus and adispatch. The three messages stay distinct: only the dimension case is a
silent wrong answer (the RHS reads as a literal coordinate and the mask
empties), so its wording has to say more than the other two.
Namespace._unknownbuilt its two kind labels and their two value liststhrough parallel ternaries that had to be kept in step by hand; one list of
pairs.
Comments
Removed the ones that restate the line below them: the per-
isinstancebranch labels in
_coerce_to_dataarray, the# Step 2:/# Step 5:numbering of a sequence that no longer exists,
# Atoms/# Unary/# Function callsover the grammar rules they name,# Referenced dimensions must be declared.Every comment carrying an argument stays — the
Keyword-not-Literalnote,the float-not-int notes, the whole rationale layer this codebase runs on.
Where a section label was doing real work it was rewritten into one rather
than deleted (
# at most one comparison, and only at the top,# NOT binds tightest, then AND, then OR).Also drops
expression_parser.RESOLVED_REFERENCE_NODES— unused, and itscomment claimed "both consumers assert on this" when neither did.
Considered and left alone
typeset/markdown's eleven forwarding methods anddata_validation's overlapwith
linopy/loaderboth look like duplication and are both argued for intheir module docstrings — the first against silent inheritance, the second
with
tests/test_data_parity.pyholding the two lanes to the same verdict.And
walk.glossaries, where only two of the threeEntrycomprehensionsshare a shape; a helper there costs two dict rebuilds to save six lines and
reads worse.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor