Repository navigation
refactor(module-graph): clear the three Sonar issues #743 left - #747
Conversation
- transitive_reduction (S3776, 18): the condensation is built in _condense, and "reachable through a third component" is _implied over a generic _reach, instead of a nested search. - build_module_graph (S3776, 16): the edge loop moves to _collect. - test_module_graph (S3415): the one assertion with its expected side first is turned round. Behaviour is unchanged: the same 25 tests pass, and query's self-drift is 0.230 (tolerance 0.24). Closes #746 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors module_graph.py by extracting helper functions _condense, _implied, and _collect to improve code organization, and updates type annotations to use PEP 695 generics. It also adjusts an assertion in the unit tests. Feedback highlights two critical issues: first, using Hashable as a type parameter bound on _reach will cause a runtime NameError because it is only imported under TYPE_CHECKING; second, the refactored _implied function introduces a performance regression by removing early short-circuiting and shared traversal state, which should be restored using an efficient helper.
Review of #747: _implied ran a separate full _reach from each successor, where the code it replaced did one DFS from all of them, so the "behaviour- preserving" refactor made the reduction up to 2.6x slower on a synthetic 2000-node / 12000-edge graph. _reach now takes several starts, and _implied walks once from all of a's other successors. Measured with the reviewer's bench against the pre-refactor function: same output on 3000 random graphs; ratio now 1.0x / 1.5x / 1.2x (cyclic 0.05, cyclic 0.01, DAG; was 1.0x / 2.6x / 1.9x). The remainder is the old early exit on reaching b, kept out to stay under Sonar's complexity limit; real module graphs have tens of nodes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/guardian review |
![codegraph-brain[bot]](https://avatars.githubusercontent.com/in/4450645?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: Checked the _reach refactor: the starts-inclusive seen set matches the old one-step-or-more reachability in _condense and _implied, and the new _implied preserves the old exclude-target logic. Checked the extraction of _collect: it moves the original loop and only changes prefix/packages/expand to scope fields, which is correct if the scope mirrors those locals. The test swap in the mermaid assertion is a harmless reordering of set equality.
🤖 claude-haiku-5-5 · 11,585 prompt + 3,193 completion = 14,778 tokens · graph 1/2 files (50%)
From guardian's review of #747: the type_only filter in the edge collection had no test. One now ingests an import under TYPE_CHECKING and expects no edge; it fails with the filter removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closes #746.
#743 merged with Sonar
new_violations = 3. I had checked that the quality gate passed but not the issue count, which is the bar.transitive_reduction(complexity 18): the condensation is built in_condense, and "reachable through a third component" is_implied, using a generic_reachinstead of a nested search.build_module_graph(complexity 16): the edge loop moves to_collect.test_module_graph.py: the one assertion with its expected side first is turned round.Behaviour is unchanged. The same 25 tests pass, self-parsing passes, and query's self-drift is 0.230 against a tolerance of 0.24.
🤖 Generated with Claude Code