#853: update Halstead and NPath metrics to improve jar handling and texting#1243
Open
marinka-aa wants to merge 1 commit into
Open
#853: update Halstead and NPath metrics to improve jar handling and texting#1243marinka-aa wants to merge 1 commit into
marinka-aa wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughHVMetric now uses a prebuilt JAR path instead of building it via Maven, raising FileNotFoundError if missing. NPathMetric adds a Maven-free fallback using AST-based computation when mvn is unavailable. Tests were updated accordingly, and skip decorators were removed from previously flaky/disabled tests. ChangesMaven-free metric execution
Test suite cleanup and config validation update
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (1)
test/config/test_config.py (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale TODO/docstring after refactor.
The docstring's TODO (referencing
#813) still says "Once all metrics meet requirements, remove the decorator," but the decorator was already removed and the fix instead filters out non-compliant metrics viametrics_excluderather than making every metric AST-compliant. The docstring now misrepresents the actual solution taken.📝 Suggested docstring update
''' - TODO `#813`:30min/DEV Ensure All Metrics Accept `ast: AST` Parameter with Type Hints - Verify that all metrics in the config accept an AST parameter with proper type hints. - Probable solutions: - 1. Every metric factory in patterns_config["metrics"] produces a metric with: - - A parameter named "ast" in its call signature - - The "ast" parameter properly annotated as `aibolit.ast_framework.ast.AST` or a subclass - 2. Remove any metrics that cannot comply with this interface. - Once all metrics meet requirements, remove the decorator. + Verify that all non-excluded metrics in the config accept an AST parameter + with proper type hints. Metrics listed in `metrics_exclude` are skipped, + mirroring the filtering done in `calculate_patterns_and_metrics_with_decomposition`. '''🤖 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/config/test_config.py` around lines 9 - 19, Update the stale docstring in test_each_metric_in_config_accepts_ast so it matches the current refactor: the test no longer relies on a decorator and now uses metrics_exclude to skip non-compliant metrics. Remove the outdated TODO language about making every metric AST-compliant and removing the decorator, and rewrite the note to describe the actual behavior of filtering out incompatible metrics in the config test.
🤖 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-63: The _value_without_maven path currently rejects directories
by only checking os.path.isfile(self.input), which breaks parity with the
Maven-based flow and produces a misleading missing-file error. Update
_value_without_maven in NPathMetric so it accepts valid directory inputs the
same way the Maven branch does, and adjust the validation/error handling to
distinguish between nonexistent paths and unsupported inputs instead of always
saying “File ... does not exist”.
---
Nitpick comments:
In `@test/config/test_config.py`:
- Around line 9-19: Update the stale docstring in
test_each_metric_in_config_accepts_ast so it matches the current refactor: the
test no longer relies on a decorator and now uses metrics_exclude to skip
non-compliant metrics. Remove the outdated TODO language about making every
metric AST-compliant and removing the decorator, and rewrite the note to
describe the actual behavior of filtering out incompatible metrics in the config
test.
🪄 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: 6b79cfad-6e02-42f0-ad93-761636bac9f8
⛔ Files ignored due to path filters (1)
test/stats/results_test.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
aibolit/metrics/hv/main.pyaibolit/metrics/npath/main.pytest/config/test_config.pytest/metrics/hv/test_all_types.pytest/metrics/npath/test_all_types.pytest/recommend/test_recommend_pipeline.pytest/stats/test_stats.py
Comment on lines
+61
to
+63
| def _value_without_maven(self): | ||
| if not os.path.isfile(self.input): | ||
| raise Exception(' '.join(['File', self.input, 'does not exist'])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether NPathMetric is ever invoked with directory-like inputs
rg -nP 'NPathMetric\(' --type=py -C3Repository: cqfn/aibolit
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## Files\n'
git ls-files | rg '^aibolit/metrics/npath/|^aibolit/metrics/|^tests/|^test/'
printf '\n## Symbol search\n'
rg -n --hidden --glob '!**/.git/**' 'NPathMetric|npath' aibolit tests . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' -C 2 || true
printf '\n## File outline\n'
ast-grep outline aibolit/metrics/npath/main.py --view expanded || trueRepository: cqfn/aibolit
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## main.py outline\n'
ast-grep outline aibolit/metrics/npath/main.py --view expanded
printf '\n## main.py excerpt\n'
sed -n '1,220p' aibolit/metrics/npath/main.py | cat -n
printf '\n## npath tests excerpt\n'
sed -n '1,140p' test/metrics/npath/test_all_types.py | cat -nRepository: cqfn/aibolit
Length of output: 15054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## npath README\n'
sed -n '1,220p' aibolit/metrics/npath/README.md | cat -n
printf '\n## NPathMetric references\n'
rg -n --hidden --glob '!**/.git/**' 'NPathMetric|MvnFreeNPathMetric|metrics/npath|npath/main.py|os\.path\.isdir\(self\.input\)' aibolit test scripts -C 3 || true
printf '\n## Metric entrypoints\n'
rg -n --hidden --glob '!**/.git/**' 'class .*Metric|def value\(self' aibolit/metrics -g '!**/*.java' -C 2 | sed -n '1,220p'Repository: cqfn/aibolit
Length of output: 14636
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## npath README\n'
sed -n '1,220p' aibolit/metrics/npath/README.md | cat -n
printf '\n## NPathMetric references\n'
rg -n --hidden --glob '!**/.git/**' 'NPathMetric|MvnFreeNPathMetric|metrics/npath|npath/main.py|os\.path\.isdir\(self\.input\)' aibolit test scripts -C 3 || true
printf '\n## Metric entrypoints\n'
rg -n --hidden --glob '!**/.git/**' 'class .*Metric|def value\(self' aibolit/metrics -g '!**/*.py' -C 2 | sed -n '1,220p'Repository: cqfn/aibolit
Length of output: 7650
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## build_ast definition\n'
rg -n --hidden --glob '!**/.git/**' 'def build_ast\(' aibolit -C 4 || true
printf '\n## build_ast usage in npath\n'
rg -n --hidden --glob '!**/.git/**' 'build_ast\(self\.input\)|build_ast\(' aibolit/metrics/npath aibolit/utils test -C 3 || trueRepository: cqfn/aibolit
Length of output: 50369
Mirror the Maven path for directories aibolit/metrics/npath/main.py:61-64
_value_without_maven only accepts files, so a directory input that the Maven branch handles is rejected with a misleading “File ... does not exist” error when mvn is unavailable.
🤖 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 - 63, The _value_without_maven
path currently rejects directories by only checking os.path.isfile(self.input),
which breaks parity with the Maven-based flow and produces a misleading
missing-file error. Update _value_without_maven in NPathMetric so it accepts
valid directory inputs the same way the Maven branch does, and adjust the
validation/error handling to distinguish between nonexistent paths and
unsupported inputs instead of always saying “File ... does not exist”.
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