#161: Fix class variable and tests#1229
Open
marinka-aa wants to merge 7 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds two new AST-based pattern detectors ( ChangesPattern implementations, metric fallback, and test updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
marinka-aa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
aibolit/utils/encoding_detector.py (1)
24-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFinal
errors='replace'fallback is unreachable.
'latin-1'decoding never raises (it maps all 256 byte values), so once it is reached in the loop it always returns successfully. The trailingreturn data.decode('utf-8', errors='replace')at Line 31 is therefore dead code. Either drop'latin-1'from the candidate list to let the replacement path handle undecodable bytes, or remove the unreachable final return.🤖 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/utils/encoding_detector.py` around lines 24 - 31, The fallback in encoding_detector.decode is unreachable because the encoding loop always succeeds once 'latin-1' is tried. Update the candidate list or the fallback logic in the decode path so that the final data.decode('utf-8', errors='replace') branch is either actually reachable or removed, keeping the behavior in sync with detect_encoding_of_data and the utf-8/latin-1 fallback order.
🤖 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/metrics/npath/main.py`:
- Around line 61-73: The fallback implementation in _value_without_maven()
rejects directory inputs even though value() already supports them, causing
inconsistent behavior when mvn is unavailable. Update _value_without_maven() to
mirror the directory-handling logic used in value() by detecting directories and
resolving the Java sources/AST the same way before calling
AST.build_from_javalang and MvnFreeNPathMetric.value(). Keep the existing
file-path handling and error behavior for missing inputs, but ensure directory
analysis works in both the Maven and non-Maven paths.
In `@PATTERNS.md`:
- Around line 629-643: Add a missing P35 entry in PATTERNS.md to match the
registry in aibolit/config.py. Update the pattern catalog by adding the Class
variable section with the same title, code, and description used by the
registered P35 symbol so the public documentation stays consistent and complete.
Use the existing P34 section as the local reference point for where the new
pattern entry should be inserted.
In `@test/patterns/class_variable/test_class_variable.py`:
- Around line 26-62: The multiline source fixtures in test_class_variable.py use
triple double-quoted strings, which triggers Ruff Q001 under the repo’s quoting
rule. Update the three string literals passed into _find_lines in
test_ignore_interface_variable, test_ignore_primitive_variable, and the
preceding fixture block to use triple single quotes instead, keeping the content
unchanged and consistent with the existing test helper calls.
---
Nitpick comments:
In `@aibolit/utils/encoding_detector.py`:
- Around line 24-31: The fallback in encoding_detector.decode is unreachable
because the encoding loop always succeeds once 'latin-1' is tried. Update the
candidate list or the fallback logic in the decode path so that the final
data.decode('utf-8', errors='replace') branch is either actually reachable or
removed, keeping the behavior in sync with detect_encoding_of_data and the
utf-8/latin-1 fallback order.
🪄 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: cb2c4979-f051-41c8-92b6-6555cc9af9ff
⛔ Files ignored due to path filters (1)
test/stats/results_test.csvis excluded by!**/*.csv
📒 Files selected for processing (21)
PATTERNS.mdaibolit/config.pyaibolit/metrics/npath/main.pyaibolit/patterns/bidirect_index/bidirect_index.pyaibolit/patterns/class_inheritance/__init__.pyaibolit/patterns/class_inheritance/class_inheritance.pyaibolit/patterns/class_variable/class_variable.pyaibolit/utils/encoding_detector.pytest/config/test_config.pytest/metrics/maxDiameter/test_max_diameter.pytest/metrics/npath/test_all_types.pytest/patterns/bidirect_index/test_bidirect_index.pytest/patterns/class_inheritance/NestedInheritance.javatest/patterns/class_inheritance/OnlyImplements.javatest/patterns/class_inheritance/SimpleInheritance.javatest/patterns/class_inheritance/TypeParameterExtends.javatest/patterns/class_inheritance/test_class_inheritance.pytest/patterns/class_variable/test_class_variable.pytest/recommend/test_recommend_pipeline.pytest/stats/test_stats.pytest/utils/test_encoding_detector.py
Comment on lines
+61
to
+73
| def _value_without_maven(self): | ||
| if not os.path.isfile(self.input): | ||
| raise Exception(' '.join(['File', self.input, 'does not exist'])) | ||
| try: | ||
| ast = AST.build_from_javalang(build_ast(self.input)) | ||
| except Exception as exception: | ||
| raise Exception(f'PMDException: {exception}') from exception | ||
| complexity = MvnFreeNPathMetric(ast).value() | ||
| return { | ||
| 'data': [{'file': self.input, 'complexity': complexity}], | ||
| 'errors': [], | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=py -C3 'NPathMetric\s*\('Repository: cqfn/aibolit
Length of output: 150
Fallback rejects directory inputs that the Maven path accepts.
The value() method handles directory inputs (Lines 37–40), but _value_without_maven() raises a File ... does not exist error for them (Line 62). This creates an environment-dependent regression: directory analysis fails only when mvn is unavailable.
Add directory handling to the fallback path to match the Maven behavior.
aibolit/metrics/npath/main.py Lines: 61-73
Original code
def _value_without_maven(self):
if not os.path.isfile(self.input):
raise Exception(' '.join(['File', self.input, 'does not exist']))
try:
ast = AST.build_from_javalang(build_ast(self.input))
except Exception as exception:
raise Exception(f'PMDException: {exception}') from exception
complexity = MvnFreeNPathMetric(ast).value()
return {
'data': [{'file': self.input, 'complexity': complexity}],
'errors': [],
}
🤖 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/metrics/npath/main.py` around lines 61 - 73, The fallback
implementation in _value_without_maven() rejects directory inputs even though
value() already supports them, causing inconsistent behavior when mvn is
unavailable. Update _value_without_maven() to mirror the directory-handling
logic used in value() by detecting directories and resolving the Java
sources/AST the same way before calling AST.build_from_javalang and
MvnFreeNPathMetric.value(). Keep the existing file-path handling and error
behavior for missing inputs, but ensure directory analysis works in both the
Maven and non-Maven paths.
Comment on lines
+629
to
+643
| *** | ||
|
|
||
| *Title*: Class inheritance | ||
|
|
||
| *Code*: **P34** | ||
|
|
||
| *Description*: Once a class extends another class via `extends`, it's a pattern. | ||
|
|
||
| *Example*: | ||
|
|
||
| ```java | ||
| class MyList extends AbstractList { // here | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a P35 section to keep the pattern catalog consistent with registry.
aibolit/config.py registers P35 (Class variable) at Line 204, but PATTERNS.md currently has no P35 entry. This leaves the public pattern dictionary incomplete.
🤖 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 `@PATTERNS.md` around lines 629 - 643, Add a missing P35 entry in PATTERNS.md
to match the registry in aibolit/config.py. Update the pattern catalog by adding
the Class variable section with the same title, code, and description used by
the registered P35 symbol so the public documentation stays consistent and
complete. Use the existing P34 section as the local reference point for where
the new pattern entry should be inserted.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ruff.yml (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin Ruff to the version declared in
pyproject.toml.
.github/workflows/ruff.ymlinstallsruffwithout a version, while the repo pinsruff==0.15.19. That lets CI drift as Ruff releases change lint behavior.🤖 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 @.github/workflows/ruff.yml at line 27, The Ruff install step in the workflow is not using the repo-pinned version, which can make CI behavior drift. Update the install command in the ruff workflow to install the same Ruff version declared in pyproject.toml, and keep the workflow aligned with that pinned version so the Ruff check runs consistently.
🤖 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.
Nitpick comments:
In @.github/workflows/ruff.yml:
- Line 27: The Ruff install step in the workflow is not using the repo-pinned
version, which can make CI behavior drift. Update the install command in the
ruff workflow to install the same Ruff version declared in pyproject.toml, and
keep the workflow aligned with that pinned version so the Ruff check runs
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b36b727-3f97-45a5-97eb-f792b9a45f27
📒 Files selected for processing (5)
.github/workflows/flake8.yml.github/workflows/ruff.ymlaibolit/__main__.pyaibolit/patterns/bidirect_index/bidirect_index.pytest/patterns/class_variable/test_class_variable.py
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/flake8.yml
- aibolit/main.py
🚧 Files skipped from review as they are similar to previous changes (2)
- test/patterns/class_variable/test_class_variable.py
- aibolit/patterns/bidirect_index/bidirect_index.py
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation