Visitar URL original
fix: match exclude_files against relative paths by yangfan-yf-yf · Pull Request #463 · cpplint/cpplint · GitHub
Skip to content

fix: match exclude_files against relative paths - #463

Open
yangfan-yf-yf wants to merge 3 commits into
cpplint:developfrom
yangfan-yf-yf:fix/exclude-files-relative-path
Open

yangfan-yf-yf wants to merge 3 commits into
cpplint:developfrom
yangfan-yf-yf:fix/exclude-files-relative-path

Conversation

@yangfan-yf-yf

@yangfan-yf-yf yangfan-yf-yf commented Aug 9, 2026 •

Copy link
Copy Markdown

Fixes #254.

Summary

  • Match each exclude_files expression against the target path relative to the CPPLINT.cfg that defines it, using / on every platform.
  • Preserve same-directory matching, parent configuration traversal, and set noparent.
  • Keep percent characters in filenames, configuration paths, and patterns literal when printing exclusion diagnostics; excluded paths such as percent%2f/baz.cc no longer crash in non-quiet mode.
  • Document the relative-path behavior and its compatibility boundary.

An anchored directory-only pattern such as ^bar$ should be written as ^bar/ when the intent is to exclude that subtree.

Validation

  • Windows / Python 3.12.3: full suite 244 passed, coverage 96.75% (required: 95%). This includes 13 configuration tests, with six cases covering percent characters and quiet/non-quiet output.
  • Independently reproduced the non-quiet percent%2f/baz.cc CLI crash on the previous head. After the fix, the same command exits successfully with the complete literal diagnostic; --quiet exits successfully without output.
  • Mypy and Pylint passed.
  • Pre-commit checks on the three changed files passed, including Ruff check/format and Codespell; git diff --check passed.
  • CI run 34791663185 passed for this head combined with develop: Ubuntu, macOS, and Windows, each on Python 3.10 and 3.14. All six jobs ran 244 tests successfully, with 96.75% coverage, and passed Pylint and Mypy. The remote pre-commit check also passed.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9181cc5d-29b0-48d2-9259-b212c236b06c

📥 Commits

Reviewing files that changed from the base of the PR and between db3dfc9 and d736a62.

📒 Files selected for processing (3)
  • CHANGELOG.rst
  • cpplint.py
  • cpplint_unittest.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • CHANGELOG.rst
  • cpplint.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

exclude_files patterns now match source paths relative to their CPPLINT.cfg, using forward slashes. Diagnostics preserve literal percent characters. Tests cover nested paths, inheritance, and set noparent.

Changes

Relative exclude_files matching

Layer / File(s) Summary
Compute and apply relative exclusion paths
cpplint.py, CHANGELOG.rst
cpplint.py builds configuration-relative paths, matches exclude_files against those paths, and reports the matching path and pattern. The changelog documents the behavior and diagnostic formatting.
Validate configuration overrides
cpplint_unittest.py
Tests cover exact, nested, nonmatching, and directory-boundary patterns, literal percent characters, inherited exclusions, and set noparent.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: walidbi200, pnhd

Merge Risk: ⚪ Minimal · up to d736a

The relative exclusion matching change and its inheritance, quiet-mode, and diagnostic behavior are covered by the provided implementation and test context. No merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #254 requires exclude_files to match a specific file in a subdirectory of the configuration directory. ProcessConfigOverrides now matches the accumulated path relative to each CPPLINT.cfg …
Out of Scope Changes check ✅ Passed The implementation, path-related diagnostic handling, focused tests, and documentation support the exclude_files path-matching change in #254. The reviewed summary shows no unrelated product or repo…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: matching exclude_files patterns against relative paths.
Full details: Docstring Coverage

Explanation

Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@yangfan-yf-yf
yangfan-yf-yf marked this pull request as ready for review August 9, 2026 17:03

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@cpplint.py`:
- Around line 270-273: Document in the exclude_files path-matching guidance in
cpplint.py (lines 270-273) that anchored directory patterns must be changed from
^bar$ to ^bar/. Add ("bar/baz.cc", "^bar$", True) to the relevant compatibility
test cases in cpplint_unittest.py (lines 7295-7302) to lock the behavior that
^bar$ does not exclude the bar subtree.
🪄 Autofix

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 Plus

Run ID: 68156488-6931-42bb-9d53-40dde9ee4fd0

📥 Commits

Reviewing files that changed from the base of the PR and between 96db7ae and 5f7c736.

📒 Files selected for processing (3)
  • CHANGELOG.rst
  • cpplint.py
  • cpplint_unittest.py

Comment thread cpplint.py Resolved

@PNHD PNHD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head db3dfc9. The implementation consistently matches exclude_files against the path relative to the defining CPPLINT.cfg, normalizes separators, preserves parent traversal and set noparent, and documents/tests the anchored-directory compatibility boundary (^bar$ vs ^bar/). The prior review finding about that boundary is addressed on this head. The PR reports 7 focused configuration tests and 238 full-suite tests passing, together with Ruff, Mypy, Pylint, Codespell, and git diff --check. LGTM.

@walidbi200 walidbi200 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head db3dfc9. The relative-path accumulation itself looks correct, including nested configs, parent traversal, set noparent, and the documented ^bar$ / ^bar/ behavior. The focused and full suites also pass.

I did find one case in the newly supported full-path matching that crashes instead of excluding the file. If a matched path contains %, the non-quiet exclusion message is later passed through % string formatting, so percent sequences that came from the filename can be interpreted as format directives.

For example, with a file at percent%2f/baz.cc and:

exclude_files=^percent%2f/baz[.]cc$

this PR matches the new relative path as intended, but then exits with TypeError: must be real number, not str while printing the exclusion notice. With --quiet it excludes correctly.

The formatting weakness already exists in the old message path, but this PR makes it reachable through the newly supported nested-path matching and also changes that diagnostic. I think it would be worth making the message formatting safe and adding a non-quiet %-path regression before merge.

Other than that, the relative-path behavior held up in my testing.

@yangfan-yf-yf

Copy link
Copy Markdown
Author

Fixed in d736a62.

I reproduced the non-quiet percent%2f/baz.cc example on the previous head: it exited with TypeError, while --quiet succeeded. The exclusion message now uses f-strings throughout, so interpolated filenames and configuration paths cannot become % format directives.

I added six regression cases covering percent characters in the target path, configuration directory, and pattern, with both quiet and non-quiet output checks. The same CLI example now exits successfully and preserves the complete literal diagnostic; quiet mode remains silent.

Validation on Windows / Python 3.12.3: 244 tests passed, coverage 96.75%; Mypy, Pylint, pre-commit on the three changed files, and git diff --check passed. The changelog and PR description are updated. Thanks for catching this.

@walidbi200 walidbi200 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head d736a62. My previous %-path blocker is fixed.

I reproduced the original percent%2f/baz.cc failure on the previous head and confirmed it now exits cleanly while preserving the literal diagnostic. The new regression coverage exercises percent characters in the target path, config directory, and pattern in both quiet and non-quiet modes.

I also checked the focused config tests, full suite, pylint, mypy, pre-commit, and git diff --check; all passed locally. No remaining blocker from me on this head. LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow exclude_files option in the config file to exclude a specific file in a subdirectory

3 participants