fix: remove a stale type: ignore and document value_eq's broad catches - #421
Open
tschm wants to merge 2 commits into
Open
fix: remove a stale type: ignore and document value_eq's broad catches#421tschm wants to merge 2 commits into
tschm wants to merge 2 commits into
Conversation
The `# type: ignore[arg-type]` on the `**options` splat no longer suppressed anything -- `ty` reported it as an unused blanket directive. A stale ignore is worse than none, since it would keep hiding a real `arg-type` error if one were reintroduced at that call site. `make typecheck` now reports 0 diagnostics where it previously found 1. Refs janushendersonassetallocation#418 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both broad catches in `value_eq` returned False with no stated reason, which is the shape a reader is trained to distrust -- every other broad catch in the codebase already carries its rationale. The asymmetry is what makes swallowing correct here. `value_eq` has one caller, `Computation.insert`, which uses it only to skip a redundant write on an UPTODATE node, so "not equal" means the value is written and descendents marked STALE: a recomputation that was not needed, rather than a stale graph that looks up to date. Letting the exception escape would instead fail an insert over a comparison the caller never asked to be able to fail. Refs janushendersonassetallocation#420 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Two quick fixes from the quality assessment in #414, one per commit.
fix(types)— remove a stale suppression (#418)src/loman/ui/widget.py:352carried# type: ignore[arg-type]on the**optionssplat, andtyflagged it as an unused blanket directive — the underlying signature has been tightened since it was added. A stale ignore is worse than none, because it keeps hiding a realarg-typeerror if one is reintroduced at that call site.make typecheckreported "Found 1 diagnostic" before this change and "All checks passed!" after.docs(util)— say whyvalue_eqswallows comparison errors (#420)Both broad catches in
value_eqreturnedFalsewith no stated reason. The intent is defensible, but it was unwritten — and every other broad catch in the codebase already carries its rationale (computeengine.py:692logs viaLOG.exception;transformer.py:1049is an optional-extra guard;computation.py:831has apragmanote;ui/value.py:81explains that a broken__repr__must not break the detail panel). These two were the exception.The rationale comes from the call site rather than from guessing at intent.
value_eqhas exactly one caller —Computation.insertatcomputeengine.py:1140— which uses it only to skip a redundant write on anUPTODATEnode. So "not equal" means the value is written and descendents markedSTALE: a recomputation that was not needed, rather than a stale graph that looks up to date. That asymmetry is what makes swallowing correct here, and letting the exception escape would instead fail an insert over a comparison the caller never asked to be able to fail.No behaviour change in either commit — one deleted comment and two added ones.
Verification
make fmtmake typecheckmake testCloses #418
Closes #420