Skip to content

Require hook name with rapids-pre-commit-hooks: directives - #177

Merged
rapids-bot[bot] merged 1 commit into
rapidsai:mainfrom
KyleFromNVIDIA:no-directive-without-check
Oct 20, 2026
Merged

rapids-bot[bot] merged 1 commit into
rapidsai:mainfrom
KyleFromNVIDIA:no-directive-without-check

Conversation

@KyleFromNVIDIA

Copy link
Copy Markdown
Member

Allowing the hook name to be omitted with these directives was a terrible idea. Require it, and update the tests and documentation accordingly.

At the time of this PR, there are very few places where the directive is used without the check name, and this change would not disrupt any of them.

Allowing the hook name to be omitted with these directives was a
terrible idea. Require it, and update the tests and documentation
accordingly.

At the time of this PR, there are very few places where the directive
is used without the check name, and this change would not disrupt any of
them.
@KyleFromNVIDIA
KyleFromNVIDIA requested a review from a team as a code owner October 20, 2026 18:03
@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: rapidsai/pre-commit-hooks/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: d07e4660-eed0-45f0-a6d5-da45f4e1768f
📥 Commits

Reviewing files that changed from the base of the PR and between bbeb2da and cc804ee.

📒 Files selected for processing: 6
  • README.md
  • src/rapids_pre_commit_hooks/hardcoded_version.py
  • src/rapids_pre_commit_hooks/lint.py
  • tests/rapids_pre_commit_hooks/test_copyright.py
  • tests/rapids_pre_commit_hooks/test_hardcoded_version.py
  • tests/rapids_pre_commit_hooks/test_lint.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Documentation

    • Clarified that suppression directives must include a non-empty, comma-separated list of checks.
    • Directives without a check list are ignored, and directives affect only the checks explicitly listed.
  • Behavior Changes

    • Lint suppression directives now use check-specific syntax, such as [test] or [other,test].
    • Hardcoded-version suppression comments now explicitly target the verify-hardcoded-version check.
    • Enable, disable, and next-line directives consistently apply only to their specified checks.

Suppression directives now require a non-empty bracketed list of check names. The linter applies directives only when that list includes the configured warning name. Documentation, suppression examples, and directive-boundary tests were updated.

Changes: Suppression Directive Filtering

Layer / File(s) Summary
Parse and filter check-scoped directives
src/rapids_pre_commit_hooks/lint.py, tests/rapids_pre_commit_hooks/test_lint.py, tests/rapids_pre_commit_hooks/test_copyright.py
Directive parsing requires a bracketed warning-name list. Filtering checks whether the configured warning name is listed. Tests cover filtered directives, malformed forms, and directive boundaries.
Update directive guidance and examples
README.md, src/rapids_pre_commit_hooks/hardcoded_version.py, tests/rapids_pre_commit_hooks/test_hardcoded_version.py
The README documents the required check list. The hardcoded-version suppression example and test expectation identify the specific check.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: jameslamb, bdice

Merge Risk: ⚪ Minimal · up to cc804

Directives now target named checks, with no identified issue that needs resolution before merge.

Pre-merge checks: Passed 4
Check name Status Explanation
Description check Passed The description directly explains the directive-format change and the related test and documentation updates.
Title check Passed The title clearly and concisely states the main change: requiring the hook name in rapids-pre-commit-hooks: directives.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@jameslamb jameslamb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I support this. We keep adding hooks here, this extra verbosity is worthwhile to avoid missing things.

@KyleFromNVIDIA

Copy link
Copy Markdown
Member Author

/merge

@rapids-bot
rapids-bot Bot merged commit 5393fd5 into rapidsai:main Oct 20, 2026
4 checks passed
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.

2 participants