Recorded from the coupling-metric review of the open upstream PRs. No action required now — this is a readability improvement to make whenever we are next editing that region, not a defect.
Context
An automated reviewer flagged walk_calls() on upstream PRs 2505 and 2506 as an efferent-coupling regression (14 callees). Audited against the base commit:
|
base 07b9143d |
PR head |
| definition |
engine.py:4201 |
engine.py:4616 |
| span |
4201-4777 (577 lines) |
4616-5349 (734 lines) |
| distinct project callees |
13 |
14 |
Our diff inside that span is +163 / −6, and the callee set difference is exactly one symbol (_php_name_text). So the metric flag is a threshold crossing, not a structural change — and the function was already 577 lines before we touched it. walk_calls is a nested function inside _extract_generic (engine.py:2691), the multi-language call-walking recursion.
The improvement worth making
engine.py:4808-4847 on the PR head — the ~40-line block deriving member_receiver / php_inline_new_type / php_inline_new_qualified from a member_call_expression — is a pure function of (node, source). It is dense tree-sitter node-shape trivia: variable_name vs member_access_expression vs parenthesized_expression, the unfielded object_creation_expression scan, the anonymous-class edge case, and the _PHP_NON_CONCRETE_TYPE_NAMES refusal.
Extracting it to module scope alongside the other _php_* helpers:
def _php_member_receiver(node, source: bytes) -> tuple[str | None, str | None, str | None]:
"""(receiver, inline_new_type, inline_new_qualified) for a member_call_expression."""
- drops
walk_calls from 734 to roughly 695 lines
- makes that logic directly unit-testable, where today it is only reachable through a full extraction run
What it does NOT do
It does not clear the coupling flag. _php_name_text is called exactly once in the whole walk_calls span (head line 4839) — inside that very block — while _read_text and next are used elsewhere and would stay. So the extraction swaps _php_name_text for _php_member_receiver: still 14 callees. If the goal were turning the gate green, this is not the change that does it.
Recommendation
Do not open a PR solely for this, and do not amend 2505/2506 for it — they are open and unreviewed upstream, and rewriting them for a metric that would not go green anyway is churn. Pick it up when we next have reason to edit that region, and land it fork-first as usual.
Recorded from the coupling-metric review of the open upstream PRs. No action required now — this is a readability improvement to make whenever we are next editing that region, not a defect.
Context
An automated reviewer flagged
walk_calls()on upstream PRs 2505 and 2506 as an efferent-coupling regression (14 callees). Audited against the base commit:07b9143dengine.py:4201engine.py:4616Our diff inside that span is +163 / −6, and the callee set difference is exactly one symbol (
_php_name_text). So the metric flag is a threshold crossing, not a structural change — and the function was already 577 lines before we touched it.walk_callsis a nested function inside_extract_generic(engine.py:2691), the multi-language call-walking recursion.The improvement worth making
engine.py:4808-4847on the PR head — the ~40-line block derivingmember_receiver/php_inline_new_type/php_inline_new_qualifiedfrom amember_call_expression— is a pure function of(node, source). It is dense tree-sitter node-shape trivia:variable_namevsmember_access_expressionvsparenthesized_expression, the unfieldedobject_creation_expressionscan, the anonymous-class edge case, and the_PHP_NON_CONCRETE_TYPE_NAMESrefusal.Extracting it to module scope alongside the other
_php_*helpers:walk_callsfrom 734 to roughly 695 linesWhat it does NOT do
It does not clear the coupling flag.
_php_name_textis called exactly once in the wholewalk_callsspan (head line 4839) — inside that very block — while_read_textandnextare used elsewhere and would stay. So the extraction swaps_php_name_textfor_php_member_receiver: still 14 callees. If the goal were turning the gate green, this is not the change that does it.Recommendation
Do not open a PR solely for this, and do not amend 2505/2506 for it — they are open and unreviewed upstream, and rewriting them for a metric that would not go green anyway is churn. Pick it up when we next have reason to edit that region, and land it fork-first as usual.