Extract NPath report parser#1231
Open
somepatt wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthrough
ChangesNPath report parsing refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 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 |
![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)
test/metrics/npath/test_report_parsing.py (1)
9-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a malformed-violation test case for parser hardening.
Current tests miss invalid/variant
<violation>text, which is the fragile branch inparse_report. A single negative test here would lock in expected failure 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 `@test/metrics/npath/test_report_parsing.py` around lines 9 - 39, The current NPathMetric.parse_report tests cover the happy path and PMD error handling, but they do not exercise malformed or variant <violation> text in the XML parser branch. Add a negative test in test_parse_report_reads_complexity_from_xml/test_parse_report_raises_pmd_error style that feeds parse_report a bad or non-matching violation message and asserts the expected failure behavior, using NPathMetric.parse_report as the target to harden its parsing logic.
🤖 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 84-91: The _relative_source_name helper is matching the generated
source prefix against raw path strings, which can break when path separators
differ. Normalize both the input path and the root-derived prefix before doing
the prefix search in _relative_source_name, then trim using the normalized match
so the function works consistently across OS path formats.
- Around line 54-56: Guard the cleanup in the main flow so `finally` does not
reference an uninitialized `root`. Initialize `root` to `None` before the `try`
block in `main`, and in the `finally` section only call `shutil.rmtree(root)`
when `root` was successfully assigned. This keeps the cleanup safe without
masking the original exception.
- Around line 69-76: The violation parsing in NPathMetric should handle missing
or changed PMD message shapes instead of assuming `file.violation.string` always
contains the expected prefix. Update the parsing logic in `NPathMetric` to
safely read `file.violation.string`, verify the “NPath complexity of ” marker
exists before slicing, and skip or default when it does not. Keep the fix
localized to the loop that builds `result['data']`, and use
`NPathMetric._relative_source_name` only after the violation text has been
validated.
---
Nitpick comments:
In `@test/metrics/npath/test_report_parsing.py`:
- Around line 9-39: The current NPathMetric.parse_report tests cover the happy
path and PMD error handling, but they do not exercise malformed or variant
<violation> text in the XML parser branch. Add a negative test in
test_parse_report_reads_complexity_from_xml/test_parse_report_raises_pmd_error
style that feeds parse_report a bad or non-matching violation message and
asserts the expected failure behavior, using NPathMetric.parse_report as the
target to harden its parsing logic.
🪄 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: f80c15ef-e609-4e83-9609-9b309a4eaa29
📒 Files selected for processing (2)
aibolit/metrics/npath/main.pytest/metrics/npath/test_report_parsing.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 82-86: The `<error>` handling in `NPathMetric` is inconsistent:
`errors` is returned but never populated, and `_relative_source_name()` is
called without using its result. Update the `NPathMetric` logic around the
`<error>` loop to choose one behavior: either collect each parsed error into
`errors` and return them, or keep the immediate raise path and remove the dead
accumulator. If you keep raising, make sure the raised message uses the
normalized filename from `_relative_source_name(error['filename'], root)`
instead of discarding it.
🪄 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: 72c4a715-4682-4683-aba1-c1fc61687954
📒 Files selected for processing (2)
aibolit/metrics/npath/main.pytest/metrics/npath/test_report_parsing.py
Comment on lines
+82
to
+86
| error_tags = soup.find_all('error') | ||
| for error in error_tags: | ||
| NPathMetric._relative_source_name(error['filename'], root) | ||
| raise Exception(error['msg']) | ||
| return {'data': data, 'errors': errors} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dead errors accumulator and discarded _relative_source_name call in the <error> handler.
Two related problems here:
errorsis declared (Line 70) and returned (Line 86) but never appended to, so'errors'is always[]. The loop insteadraises on the first<error>, which also drops any subsequent errors.- Line 84 calls
NPathMetric._relative_source_name(error['filename'], root)and discards the result, so the normalized filename never reaches the raised message (which uses the rawerror['msg']).
Decide on one contract: either collect errors and return them, or raise immediately and drop the unused list/no-op call.
Option A — accumulate errors (matches the returned `errors` field)
error_tags = soup.find_all('error')
for error in error_tags:
- NPathMetric._relative_source_name(error['filename'], root)
- raise Exception(error['msg'])
+ name = NPathMetric._relative_source_name(error['filename'], root)
+ errors.append(f'{name}: {error["msg"]}')
return {'data': data, 'errors': errors}Option B — keep raising, but include the normalized name and drop the dead list
error_tags = soup.find_all('error')
for error in error_tags:
- NPathMetric._relative_source_name(error['filename'], root)
- raise Exception(error['msg'])
+ name = NPathMetric._relative_source_name(error['filename'], root)
+ raise Exception(f'{name}: {error["msg"]}')🤖 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 82 - 86, The `<error>` handling
in `NPathMetric` is inconsistent: `errors` is returned but never populated, and
`_relative_source_name()` is called without using its result. Update the
`NPathMetric` logic around the `<error>` loop to choose one behavior: either
collect each parsed error into `errors` and return them, or keep the immediate
raise path and remove the dead accumulator. If you keep raising, make sure the
raised message uses the normalized filename from
`_relative_source_name(error['filename'], root)` instead of discarding it.
Contributor
Author
|
@yegor256 please review this PR. |
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
mvnProblem
NPathMetricstill mixes process orchestration and PMD output analysis: the parser is only reachable through a private method that readstarget/pmd.xmlfrom a generated root directory. That makes the XML-analysis path awkward to test in isolation.Fix
Split the parsing path into
_parse_report_file()andparse_report().value()still drives Maven and the temporary project layout, but the report-analysis logic now accepts XML content directly, which lets tests target parsing behavior without filesystem setup or subprocess execution.Verification
TMPDIR=/Users/iliaprokofev/.tmp-codex-aibolit python3 -B -m pytest test/metrics/npath/test_report_parsing.py -qpython3 -B -m flake8 --max-line-length=120 aibolit/metrics/npath/main.py test/metrics/npath/test_report_parsing.pyTMPDIR=/Users/iliaprokofev/.tmp-codex-aibolit python3 -B -m pylint aibolit/metrics/npath/main.py test/metrics/npath/test_report_parsing.pyNotes
test/metrics/npath/test_all_types.pycases still requiremvnin the current tree, so they were not re-run in this environment.Closes #796
Summary by CodeRabbit
<error>handling, malformed violation text, and Windows-style path normalization.