Skip to content

fix: filter format suggestions with all-inclusive range#194

Merged
2bndy5 merged 1 commit into
mainfrom
exclusive-range-end
Apr 3, 2026
Merged

fix: filter format suggestions with all-inclusive range#194
2bndy5 merged 1 commit into
mainfrom
exclusive-range-end

Conversation

@2bndy5
Copy link
Copy Markdown
Contributor

@2bndy5 2bndy5 commented Apr 3, 2026 •
edited by coderabbitai Bot
Loading

Python's range() is partially inclusive ([start, end)). where clang-format's CLI arg --lines uses an all inclusive range.

Note, we already have a test that simulates the exact scenario we are trying resolve here. I just needed to adjust the validating logic so it reflects this patch's filtering logic.

refs:

Summary by CodeRabbit

  • Bug Fixes
    • Fixed boundary handling for line-range checks so formatting annotations and replacements correctly match modified code regions, including single-line and end-of-range cases, preventing missed or incorrectly applied format markers.

@github-actions github-actions Bot added the bug Something isn't working label Apr 3, 2026
@2bndy5 2bndy5 changed the title fix: special filtering of format fixes for a single changed line fix: do not filter out format suggestions about a single changed line Apr 3, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 3, 2026 •
edited
Loading

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a51d2e9-ba3a-4555-96e0-3e7c57359c3d

📥 Commits

Reviewing files that changed from the base of the PR and between 966e98c and 2862892.

📒 Files selected for processing (2)
  • cpp_linter/clang_tools/clang_format.py
  • tests/capture_tools_output/test_tools_output.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/capture_tools_output/test_tools_output.py
  • cpp_linter/clang_tools/clang_format.py

Walkthrough

Updated line-range membership checks to include the range end by changing range(r[0], r[1]) to range(r[0], r[1] + 1) in both the clang-format parsing logic and its test, ensuring lines equal to the former upper endpoint are considered within the range.

Changes

Cohort / File(s) Summary
Format replacement logic
cpp_linter/clang_tools/clang_format.py		 Changed membership check for clang-format <replacement> entries to use range(r[0], r[1] + 1) so the upper line is included.
Tests for format annotations
tests/capture_tools_output/test_tools_output.py		 Updated test_format_annotations to match the new inclusive-range semantics by using range(r[0], r[1] + 1) when asserting annotated lines fall within changed ranges.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • shenxianpeng
🚥 Pre-merge checks | ✅ 3
✅ 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 directly addresses the main change: adjusting range filtering logic to be inclusive of both endpoints, fixing the handling of format suggestions for single-line changes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch exclusive-range-end

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.

@2bndy5 2bndy5 temporarily deployed to test-coverage April 3, 2026 00:20 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.29%. Comparing base (4a64c10) to head (2862892).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cpp_linter/clang_tools/clang_format.py 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #194   +/-   ##
=======================================
  Coverage   98.29%   98.29%           
=======================================
  Files          24       24           
  Lines        1938     1938           
=======================================
  Hits         1905     1905           
  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.

Alexolut
Alexolut reviewed Apr 3, 2026
View reviewed changes
Comment thread cpp_linter/clang_tools/clang_format.py Outdated
@2bndy5
fix: filter format suggestions with all-inclusive range
2862892 
Python's `range()` is partially inclusive (`[start, end)`). where clang-format's CLI arg `--lines` uses an all inclusive range.

refs:
- cpp-linter/cpp-linter-action#416
- cpp-linter/cpp-linter-action#388

filter all suggestions with end + 1
@2bndy5 2bndy5 force-pushed the exclusive-range-end branch from 966e98c to 2862892 Compare April 3, 2026 10:42
@2bndy5 2bndy5 changed the title fix: do not filter out format suggestions about a single changed line fix: filter format suggestions with all-inclusive range Apr 3, 2026
@2bndy5 2bndy5 temporarily deployed to test-coverage April 3, 2026 10:52 — with GitHub Actions Inactive
@2bndy5 2bndy5 merged commit 87b634e into main Apr 3, 2026
41 checks passed
@2bndy5 2bndy5 deleted the exclusive-range-end branch April 3, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Alexolut Alexolut Alexolut left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@2bndy5 @Alexolut