fix: use diagnostic name as default #173
Conversation
WalkthroughThe diagnostic_link function in clang_tidy.py is refactored to relax strict assertion handling for clang-analyzer diagnostics by replacing a fixed split with guarded splits, returning the original diagnostic if it lacks expected parts. A corresponding test is added to verify this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp_linter/clang_tools/clang_tidy.py (1)
95-98: LGTM! The defensive check correctly prevents errors.The fix properly handles diagnostics without hyphens by checking the split result length before formatting. This prevents unpacking errors and gracefully falls back to returning the plain diagnostic name.
One minor edge case: if
self.diagnosticis exactly"-", it would split into['', ''](length 2) and format an invalid URL//html). However, this scenario is highly unlikely in practice since diagnostic names should be meaningful identifiers.Optional: Guard against empty parts after split
diag_split = self.diagnostic.split("-", maxsplit=1) -if len(diag_split) < 2: +if len(diag_split) < 2 or not all(diag_split): return self.diagnostic return link + "{}/{}.html)".format(*diag_split)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp_linter/clang_tools/clang_tidy.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #173 +/- ##
=======================================
Coverage 98.26% 98.26%
=======================================
Files 23 23
Lines 1897 1904 +7
=======================================
+ Hits 1864 1871 +7
Misses 33 33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
ba8e5e4 to
6151d33
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cpp_linter/clang_tools/clang_tidy.py (1)
95-98: Good defensive check for handling diagnostics without hyphens.The guard correctly prevents attempting to format a documentation link when the diagnostic name lacks a hyphen, returning the raw diagnostic name instead. This aligns well with the PR objective.
💡 Optional: Consider applying similar defensive handling to the clang-analyzer case
For consistency, you could replace the assertion at line 93 with a defensive check similar to this one:
if self.diagnostic.startswith("clang-analyzer-"): check_name_parts = self.diagnostic.split("-", maxsplit=2) - assert len(check_name_parts) > 2, "diagnostic name malformed" - return link + "clang-analyzer/{}.html)".format(check_name_parts[2]) + if len(check_name_parts) > 2: + return link + "clang-analyzer/{}.html)".format(check_name_parts[2]) + return self.diagnosticThis would ensure
TidyNotificationobjects constructed directly (outside ofparse_tidy_output) with malformed clang-analyzer diagnostics don't trigger assertion failures.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp_linter/clang_tools/clang_tidy.pycspell.config.ymltests/test_misc.pytests/test_parse_clang_tidy_stdout.py
✅ Files skipped from review due to trivial changes (1)
- cspell.config.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-05T05:49:09.136Z
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter PR: 124
File: cpp_linter/clang_tools/clang_tidy.py:145-145
Timestamp: 2024-10-05T05:49:09.136Z
Learning: In the `get_suggestions_from_patch` method in `cpp_linter/clang_tools/clang_tidy.py`, it is acceptable to use `assert` statements for type checking when the assertion cannot evaluate as false due to `review_comments` data being passed by reference, and when it's intended exclusively for type checking tools. In such cases, writing unit tests to cover the assertion may be unnecessary.
Applied to files:
tests/test_parse_clang_tidy_stdout.py
🧬 Code graph analysis (2)
tests/test_parse_clang_tidy_stdout.py (1)
cpp_linter/clang_tools/clang_tidy.py (1)
parse_tidy_output(279-322)
tests/test_misc.py (1)
cpp_linter/clang_tools/clang_tidy.py (1)
diagnostic_link(86-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (ubuntu-latest, 9)
- GitHub Check: test (ubuntu-latest, 12)
- GitHub Check: test (ubuntu-latest, 11)
- GitHub Check: test (ubuntu-latest, 19)
- GitHub Check: test (ubuntu-latest, 10)
- GitHub Check: test (ubuntu-latest, 14)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (ubuntu-latest, 17)
- GitHub Check: test (ubuntu-latest, 15)
- GitHub Check: test (windows-latest, 14)
- GitHub Check: test (ubuntu-latest, 16)
- GitHub Check: test (ubuntu-latest, 21)
- GitHub Check: test (windows-latest, 9)
- GitHub Check: test (windows-latest, 19)
- GitHub Check: test (ubuntu-latest, 18)
- GitHub Check: test (windows-latest, 18)
- GitHub Check: test (windows-latest, 17)
- GitHub Check: test (windows-latest, 10)
- GitHub Check: test (windows-latest, 21)
- GitHub Check: test (windows-latest, 16)
🔇 Additional comments (3)
tests/test_misc.py (1)
158-171: LGTM! Test correctly validates the defensive fallback.The test properly verifies that
diagnostic_linkreturns the raw diagnostic name when no hyphen is present. While a diagnostic containing spaces like "no diagnostic name" wouldn't be created by the updatedparse_tidy_outputlogic (which filters for no spaces), the defensive check is still valuable sinceTidyNotificationcan be constructed directly in other contexts.tests/test_parse_clang_tidy_stdout.py (1)
47-55: LGTM! Test provides good validation coverage.The test correctly validates that
parse_tidy_outputproperly:
- Parses the 4 diagnostic lines with
[clang-diagnostic-error]pattern from TIDY_OUT- Filters diagnostic names to ensure no spaces (avoiding false matches like
[with auto = ...])- Ensures diagnostic names contain hyphens (matching valid clang-tidy diagnostic format)
The assertions align well with the updated parsing logic in
cpp_linter/clang_tools/clang_tidy.py.cpp_linter/clang_tools/clang_tidy.py (1)
296-310: Excellent filtering to prevent false diagnostic matches.The addition of validation logic correctly handles edge cases where clang-tidy output includes square-bracketed context (e.g.,
[with auto = typename ...]) that shouldn't be treated as diagnostic notifications. The dual check for no spaces and containing a hyphen effectively filters out these false positives while allowing valid diagnostic names through.The explanatory comment is helpful for future maintainers.
This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs. Specifically when the diagnostic name does not satisfy any of the following conditions: - starts with `clang-diagnostic-` - starts with `clang-analyzer-` - does not contain any `-` (hyphens) at all And I added an arbitrary test for code coverage.
6151d33 to
1cd0226
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_misc.py (1)
158-171: Test correctly validates the no-hyphen case.The test properly verifies that
diagnostic_linkreturns the diagnostic name unchanged when it contains no hyphens. The logic aligns with the implementation that checkslen(diag_split) < 2after splitting on-.Consider using a more realistic diagnostic name without spaces (e.g.,
"nodashes"or"singleword") instead of"no diagnostic name". Real diagnostic names typically don't contain spaces. While the current test works correctly, a more realistic name would better represent potential edge cases.🔎 Optional refinement
@pytest.mark.no_clang def test_diagnostic_link_no_hyphen() -> None: """Test that diagnostic_link returns diagnostic name if no hyphen is present.""" note = TidyNotification( ( "test.cpp", 1, 1, "error", "Test rationale", - "no diagnostic name", + "nodashes", ) ) - assert note.diagnostic_link == "no diagnostic name" + assert note.diagnostic_link == "nodashes"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter/clang_tools/clang_tidy.pytests/test_misc.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp_linter/clang_tools/clang_tidy.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter PR: 124
File: cpp_linter/clang_tools/clang_tidy.py:145-145
Timestamp: 2024-10-05T05:49:09.136Z
Learning: In the `get_suggestions_from_patch` method in `cpp_linter/clang_tools/clang_tidy.py`, it is acceptable to use `assert` statements for type checking when the assertion cannot evaluate as false due to `review_comments` data being passed by reference, and when it's intended exclusively for type checking tools. In such cases, writing unit tests to cover the assertion may be unnecessary.
🧬 Code graph analysis (1)
tests/test_misc.py (1)
cpp_linter/clang_tools/clang_tidy.py (2)
TidyNotification(19-104)diagnostic_link(86-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: docs / build
- GitHub Check: test (ubuntu-latest, 9)
- GitHub Check: test (ubuntu-latest, 21)
- GitHub Check: test (ubuntu-latest, 13)
- GitHub Check: test (ubuntu-latest, 19)
- GitHub Check: test (windows-latest, 11)
- GitHub Check: test (ubuntu-latest, 20)
- GitHub Check: test (windows-latest, 16)
- GitHub Check: test (ubuntu-latest, 16)
- GitHub Check: test (ubuntu-latest, 18)
- GitHub Check: test (ubuntu-latest, 14)
- GitHub Check: test (ubuntu-latest, 15)
- GitHub Check: test (ubuntu-latest, 10)
- GitHub Check: test (ubuntu-latest, 12)
- GitHub Check: test (windows-latest, 9)
- GitHub Check: test (ubuntu-latest, 11)
- GitHub Check: test (windows-latest, 13)
- GitHub Check: test (windows-latest, 14)
- GitHub Check: test (windows-latest, 17)
- GitHub Check: test (windows-latest, 19)
🔇 Additional comments (1)
tests/test_misc.py (1)
156-157: LGTM!Standard Python formatting with two blank lines between test functions.
|
This is related to cpp-linter/cpp-linter-action#389 but not the solution to the actual cause. See #176 for the actual solution. |
Corresponds to cpp-linter/cpp-linter#173. This should never happen in the wild, but just in case there's an underlying problem with parsing clang-tidy output... This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs. Specifically when the diagnostic name does not satisfy the following conditions: - starts with "clang-analyzer-" - starts with "clang-diagnostic-" - contains at least 1 hyphen (`-`) This also removes the `.unwrap()` calls (bad practice) and adds a test for code coverage.
Corresponds to cpp-linter/cpp-linter#173. This should never happen in the wild, but just in case there's an underlying problem with parsing clang-tidy output... This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs. Specifically when the diagnostic name does not satisfy the following conditions: - starts with "clang-analyzer-" - starts with "clang-diagnostic-" - contains at least 1 hyphen (`-`) This also removes the `.unwrap()` calls (bad practice) and adds a test for code coverage.
This fixes a problem about hyperlinking a clang-tidy diagnostic name to the clang-tidy docs.
Specifically when the diagnostic name does not satisfy any of the following conditions:
clang-diagnostic-clang-analyzer--)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.