fix: skip parsing clang-tidy diagnostic rationale by 2bndy5 · Pull Request #176 · cpp-linter/cpp-linter

2bndy5 · 2025-12-23T19:21:08Z

Specifically when the rationale line has added context in square brackets.

This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389.

Summary by CodeRabbit

Release Notes

Bug Fixes
- Improved clang-tidy diagnostic parsing to filter contextual notes, reducing unnecessary notifications and improving output clarity.
Chores
- Added "declval" to spell-check vocabulary.
Tests
- Added test coverage for clang-tidy output parsing.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug report in cpp-linter/cpp-linter-action#389.

coderabbitai · 2025-12-23T19:24:30Z

Walkthrough

This PR modifies clang-tidy note parsing to filter which diagnostic lines become TidyNotification objects by requiring the diagnostic name to contain a dash and have no spaces. A corresponding test validates this behavior, and the spell-check vocabulary is expanded with the term "declval".

Changes

Cohort / File(s)	Summary
Clang-tidy parsing filtering `cpp_linter/clang_tools/clang_tidy.py`	Added conditional filtering to NOTE_HEADER line processing: only creates TidyNotification when diagnostic name (group 6) contains dash and has no spaces; skips bracketed contextual notes like "[with auto = typename ...]". Existing notification handling and line accumulation remain unchanged for matching diagnostics.
Test validation `tests/test_parse_clang_tidy_stdout.py`	New test file with sample clang-tidy output (TIDY_OUT) and test_parse_clang_tidy_output() that asserts resulting advice contains exactly 4 notes, each with diagnostic strings free of spaces and containing a dash character.
Spell-check configuration `cspell.config.yml`	Added "declval" to the words list to expand spell-check vocabulary.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

fix: parse clang-tidy output when WarningsAsErrors is asserted #162: Modifies clang_tidy.py note parsing to widen NOTE_HEADER pattern recognition; this PR adds filtering heuristics for diagnostic qualification in the same parsing flow.

Suggested reviewers

shenxianpeng

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: skipping parsing of clang-tidy diagnostic rationale, which aligns with the core modification in clang_tidy.py filtering NOTE_HEADER lines.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-parse-tidy-out

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e2a47a and 8c65782.

📒 Files selected for processing (3)

cpp_linter/clang_tools/clang_tidy.py
cspell.config.yml
tests/test_parse_clang_tidy_stdout.py

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-09-04T23:06:44.577Z

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter PR: 155
File: docs/conf.py:284-299
Timestamp: 2025-09-04T23:06:44.577Z
Learning: In cpp-linter project, the maintainer 2bndy5 prefers using assert statements in documentation generation code (like docs/conf.py) rather than safe guards, as they want the doc generation to fail fast if expected conditions aren't met. This ensures complete CLI documentation is produced.

Applied to files:

cspell.config.yml

📚 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 (1)

tests/test_parse_clang_tidy_stdout.py (1)

cpp_linter/clang_tools/clang_tidy.py (1)

parse_tidy_output (276-319)

⏰ 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). (2)

GitHub Check: Codacy Static Code Analysis
GitHub Check: pre-commit / Check PR title

🔇 Additional comments (5)

cspell.config.yml (1)

17-17: LGTM!

The addition of "declval" (C++ standard library utility) to the spell-check vocabulary is appropriate and aligns with its appearance in the test output sample.

tests/test_parse_clang_tidy_stdout.py (2)

4-44: LGTM!

The sample clang-tidy output is well-constructed and includes the actual bug scenario reported in cpp-linter/cpp-linter-action#389. It contains both valid diagnostics (lines 5, 17, 26, 29 with [clang-diagnostic-error]) and contextual rationale lines that should be filtered (line 41 with [with file:auto = ...] containing spaces).

47-55: LGTM!

The test function correctly validates the new filtering behavior:

Expects exactly 4 notes (corresponding to valid [clang-diagnostic-error] diagnostics)

Verifies each diagnostic has no spaces and contains a dash

Properly marked with @pytest.mark.no_clang since it's a pure parsing test

The test expectations align perfectly with the filtering logic implemented in cpp_linter/clang_tools/clang_tidy.py.

cpp_linter/clang_tools/clang_tidy.py (2)

293-297: LGTM!

The filtering logic correctly addresses the reported bug:

Clear explanatory comment with concrete example

Extracts diagnostic name from the regex match

Two-part filter: no spaces (excludes contextual rationale) AND contains dash (ensures valid diagnostic format)

This is a targeted, minimal fix that solves the parsing issue without affecting valid diagnostics.

298-307: LGTM!

The TidyNotification creation logic is correctly moved inside the conditional block. The existing logic remains unchanged—only the indentation has been adjusted to make notification creation conditional on the filtering criteria. This ensures that only valid diagnostic lines create notification objects.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2025-12-23T20:34:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.27%. Comparing base (7e2a47a) to head (8c65782).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
+ Coverage   98.26%   98.27%   +0.01%     
==========================================
  Files          23       24       +1     
  Lines        1897     1909      +12     
==========================================
+ Hits         1864     1876      +12     
  Misses         33       33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389. Corresponds to cpp-linter/cpp-linter#176

2bndy5 · 2025-12-29T16:47:25Z

Te way this is written, I can't imagine how this would cause regressions. Moving forward with this.

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug reported in cpp-linter/cpp-linter-action#389. Corresponds to cpp-linter/cpp-linter#176

github-actions bot added the bug Something isn't working label Dec 23, 2025

fix: skip parsing clang-tidy diagnostic rationale

8c65782

Specifically when the rationale line has added context in square brackets. This includes a test based on the actual bug report in cpp-linter/cpp-linter-action#389.

2bndy5 force-pushed the fix-parse-tidy-out branch from 990883a to 8c65782 Compare December 23, 2025 19:24

2bndy5 mentioned this pull request Dec 23, 2025

fix: use diagnostic name as default #173

Merged

2bndy5 mentioned this pull request Dec 23, 2025

fix: skip parsing clang-tidy diagnostic rationale cpp-linter/cpp-linter-rs#237

Merged

2bndy5 merged commit a846191 into main Dec 29, 2025
43 checks passed

2bndy5 deleted the fix-parse-tidy-out branch December 29, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: skip parsing clang-tidy diagnostic rationale#176

fix: skip parsing clang-tidy diagnostic rationale#176
2bndy5 merged 1 commit intomainfrom
fix-parse-tidy-out

2bndy5 commented Dec 23, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 23, 2025 •

edited

Loading

Uh oh!

codecov bot commented Dec 23, 2025

Uh oh!

2bndy5 commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

2bndy5 commented Dec 23, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

codecov bot commented Dec 23, 2025

Codecov Report

Uh oh!

2bndy5 commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

2bndy5 commented Dec 23, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 23, 2025 •

edited

Loading