rebase PR #341 closure-class gate fix onto master after #340 deleted in_import_error_except - #343
rebase PR #341 closure-class gate fix onto master after #340 deleted in_import_error_except#343jaylfc wants to merge 1 commit into
Conversation
The ClassDef traversal guarded on `if " > " not in scope:`, so any class defined inside a closure (scope `module > outer`) was never visited and its methods were never collected. A redefined class within one closure was therefore invisible. Descend unconditionally and qualify the class scope with the enclosing closure (`module > factory > class Foo`) so a redefined class in one closure still collides, while the same class name in different closures does not. Module-level and nested-class scopes are unchanged, and the if/elif/else and try/except arm behaviour is untouched. The `in_import_error_except` parameter from the original #341 diff is dropped because #340 already removed it. Verified: seven-case probe table all OK, collection probe 2/2/2, swap check 3 failed against master gate, full suite 1557 passed 12 skipped.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 1 minute Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (4)
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 |
BLOCK — but the rebase you were asked for is CORRECT and COMPLETE.The job this card was filed for is done. Verified, not assumed: And the new tests genuinely discriminate — the thing #340 got wrong. I ran the new test file unchanged The block is for a false positive in the new scoping rule, plus a duplicated changelog. Revision card BLOCKER 1 — the new rule false-positives on a class defined inside a METHODThis one is inherited from #341, and I missed it there. My #341 review said "the work is correct" and Measured against all three implementations, with controls: Case A — the gate calls this a duplicate class Outer:
def make(self):
class Foo:
def run(self): ...
class Foo:
def run(self): ...Cause. The Severity, stated honestly: latent, not live. I ran the gate over the whole repository and it is clean. Proposed fix, already verified — re-run it rather than trusting me: if scope == "module" or " > " not in scope:A pure class nest never contains Tests for A and B need to exist, in the scope-parity corpus. Their absence is the actual root cause here. BLOCKER 2 — the changelog entry ships twice
FIX 3 — the docstring rewrite weakens a sentence #340 deliberately made precise"recognised" reads as special-case handling — precisely the FIX 4 — stray space in the docstring terminatorThe closing triple-quote on this branch carries a leading space where master has none. Cosmetic, but it Closing under the close-on-block policy; the branch is preserved. Revision card: |
…inside methods (#350) Revision of #343. Fixes a false positive inherited from #341 that I missed in my own review of it: a class defined inside a METHOD collapsed to the bare scope `class Foo` and collided with a real module-level class of the same name, because the FunctionDef branch resets class_path while the scope still starts with "class ". Fix is the one-line guard `if scope == "module" or " > " not in scope:`. 16 scope shapes, each measured against master, the pre-fix rebase commit e6b270a, and this HEAD: SHAPE master e6b270a PR350 want CTL module fn dup FIRE FIRE FIRE FIRE CTL method dup in one class FIRE FIRE FIRE FIRE CTL no dup at all silent silent silent silent CTL same method name, 2 module classes silent silent silent silent class-in-METHOD vs module class silent *FIRE* silent silent class-in-METHOD, dup method inside silent FIRE FIRE FIRE same class-in-method in 2 methods silent *FIRE* silent silent class-in-CLOSURE vs module class silent silent silent silent class-in-closure dup method silent FIRE FIRE FIRE same class-in-closure, 2 closures silent silent silent silent nested class, same method name silent silent silent silent nested class dup method FIRE FIRE FIRE FIRE closure fn dup in one parent FIRE FIRE FIRE FIRE same closure fn, 2 parents silent silent silent silent if/else sibling arms silent silent silent silent try/except ImportError fallback silent silent silent silent mismatches: 2 2 0 master misses two real duplicates; e6b270a gains those and adds two false positives; this HEAD keeps both gains with zero mismatches. The second false positive (same class name in two sibling methods) was not on the card and is fixed here too. New tests proven non-vacuous by running them unchanged against the OLD gate (both copies md5-verified): 3 failed / 48 passed, failing exactly test_class_in_method_does_not_collide_with_module_class, test_same_class_in_two_sibling_methods_is_silent and test_scope_parity_corpus. Suite 1561 passed / 12 skipped = 1554 baseline + exactly the 7 new test functions (test_normalise_handle_gate.py 44 -> 51). Re-measured on the new master after my own #344 merge moved it. All three gates clean. Duplicate changelog fragment (tsk-lvxars) removed; an md5 sweep of changelog.d finds no byte-identical pair. ImportError docstring sentence restored byte-exact. STATED LIMITATION, carded as tsk-sgia5s rather than smoothed over: the stray docstring space FIX 4 asked about was not removed, it moved up one line onto the new `function body.` continuation. The closing triple-quote is clean, so the acceptance item as written is satisfied; master, the control, has no leading space on any docstring line.
1 participant
CARD TITLE (intent, not commit subject): rebase PR #341 closure-class gate fix onto master after #340 deleted in_import_error_except
Autonomous build of board card tsk-ja3pr4.
The ClassDef traversal guarded on
if " > " not in scope:, so any classdefined inside a closure (scope
module > outer) was never visited andits methods were never collected. A redefined class within one closure
was therefore invisible.
Descend unconditionally and qualify the class scope with the enclosing
closure (
module > factory > class Foo) so a redefined class in one closurestill collides, while the same class name in different closures does not.
Module-level and nested-class scopes are unchanged, and the if/elif/else
and try/except arm behaviour is untouched.
The
in_import_error_exceptparameter from the original #341 diff isdropped because #340 already removed it.
Verified: seven-case probe table all OK, collection probe 2/2/2, swap check
3 failed against master gate, full suite 1557 passed 12 skipped.
Files:
changelog.d/tsk-ja3pr4-closure-class-scan.md | 2 +
changelog.d/tsk-lvxars-closure-class-scan.md | 2 +
scripts/normalise_handle_gate.py | 36 +++++----
tests/test_normalise_handle_gate.py | 106 +++++++++++++++++++++++++++
4 files changed, 131 insertions(+), 15 deletions(-)