Deduplicate class-level patterns after decomposition#1232
Conversation
📝 WalkthroughWalkthroughThe decomposition pipeline now treats selected pattern codes as class-level during per-component analysis, reuses a cached class-level result for them, and filters excluded patterns and metrics before iterating components. A new regression test checks single-count reporting for ChangesClass-level pattern handling
Regression coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
🧹 Nitpick comments (1)
test/recommend/test_recommend_pipeline.py (1)
172-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover all cached class-level pattern codes in the regression.
CLASS_LEVEL_PATTERN_CODESalso includesP7andP9, but this test only guardsP4/P24. Please add fixture coverage/assertions for the remaining class-level codes, and mirror the first-component line assertion forP24.Small assertion to add for the existing fixture
self.assertEqual(sum(component['input_params']['P24'] for component in components), 1) self.assertEqual(len(components[0]['code_lines_dict']['lines_P4']), 1) + self.assertEqual(len(components[0]['code_lines_dict']['lines_P24']), 1)🤖 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 `@test/recommend/test_recommend_pipeline.py` around lines 172 - 180, The regression test in test_recommend_pipeline should cover all cached class-level pattern codes, not just P4 and P24. Update the existing assertions around components to also include P7 and P9 from CLASS_LEVEL_PATTERN_CODES, and add fixture coverage so the test verifies their input_params and code_lines_dict behavior across components. Also mirror the first-component line assertion for P24 so the test checks its lines list in the same way as P4.
🤖 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 `@aibolit/__main__.py`:
- Around line 293-297: Class-level patterns are being computed in __main__.py
before suppression is applied, so suppressed codes can still trigger unnecessary
work or affect decomposition. Update the class-level pattern evaluation in the
comprehension that builds class_level_pattern_results to skip any pattern
already suppressed, using the same suppression state checked later in the
decomposition flow, and keep the logic aligned with the pattern code symbols in
patterns_info and CLASS_LEVEL_PATTERN_CODES.
---
Nitpick comments:
In `@test/recommend/test_recommend_pipeline.py`:
- Around line 172-180: The regression test in test_recommend_pipeline should
cover all cached class-level pattern codes, not just P4 and P24. Update the
existing assertions around components to also include P7 and P9 from
CLASS_LEVEL_PATTERN_CODES, and add fixture coverage so the test verifies their
input_params and code_lines_dict behavior across components. Also mirror the
first-component line assertion for P24 so the test checks its lines list in the
same way as P4.
🪄 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
Run ID: 077ca4c0-f1f8-406a-99c0-e07ad4d64213
📒 Files selected for processing (2)
aibolit/__main__.pytest/recommend/test_recommend_pipeline.py
| class_level_pattern_results = { | ||
| pattern_info['code']: pattern_info['make']().value(class_ast) | ||
| for pattern_info in patterns_info | ||
| if pattern_info['code'] in CLASS_LEVEL_PATTERN_CODES | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect suppression before precomputing class-level patterns.
Suppressed class-level patterns are still evaluated here before Line 304 can zero them out, so a suppressed P4/P7/P9/P24 can still fail the whole decomposition or waste work.
Proposed fix
class_level_pattern_results = {
pattern_info['code']: pattern_info['make']().value(class_ast)
for pattern_info in patterns_info
- if pattern_info['code'] in CLASS_LEVEL_PATTERN_CODES
+ if (
+ pattern_info['code'] in CLASS_LEVEL_PATTERN_CODES
+ and pattern_info['code'] not in patterns_to_suppress
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class_level_pattern_results = { | |
| pattern_info['code']: pattern_info['make']().value(class_ast) | |
| for pattern_info in patterns_info | |
| if pattern_info['code'] in CLASS_LEVEL_PATTERN_CODES | |
| } | |
| class_level_pattern_results = { | |
| pattern_info['code']: pattern_info['make']().value(class_ast) | |
| for pattern_info in patterns_info | |
| if ( | |
| pattern_info['code'] in CLASS_LEVEL_PATTERN_CODES | |
| and pattern_info['code'] not in patterns_to_suppress | |
| ) | |
| } |
🤖 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 `@aibolit/__main__.py` around lines 293 - 297, Class-level patterns are being
computed in __main__.py before suppression is applied, so suppressed codes can
still trigger unnecessary work or affect decomposition. Update the class-level
pattern evaluation in the comprehension that builds class_level_pattern_results
to skip any pattern already suppressed, using the same suppression state checked
later in the decomposition flow, and keep the logic aligned with the pattern
code symbols in patterns_info and CLASS_LEVEL_PATTERN_CODES.
|
@yegor256 please review this PR. |
1 participant
Summary
P4/P24findingsProblem
calculate_patterns_and_metrics_with_decomposition()runs every pattern on every decomposed component. Patterns that inspectCLASS_DECLARATIONtherefore report the same class once per component, which duplicates findings and distorts ranking.Fix
Precompute class-level pattern results on the whole class AST and attach them only to the first decomposed component. Other components keep an empty result for those patterns.
Verification
python3 -B -m pytest test/recommend/test_recommend_pipeline.py -qpython3 -B -m flake8 --max-line-length=120 aibolit/__main__.py test/recommend/test_recommend_pipeline.pyCloses #1217
Summary by CodeRabbit