Skip to content

ci: DRY ruff calls #5267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ulgens wants to merge 3 commits into
base: master
Choose a base branch
from
+10 −3
Open

ci: DRY ruff calls #5267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec6478b
Bump ruff-pre-commit
ulgens Dec 26, 2025
8fde213
Try to centralize include setting for ruff
ulgens Dec 26, 2025
6037f2a
Make use of existing pre-commit setup in tox testenv:linters
ulgens Dec 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
# ruff-pre-commit is not using tool.ruff.include setting,
# so `files` need to be specified here.
# https://github.com/astral-sh/ruff-pre-commit/issues/71
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.13.2
rev: v0.14.10
hooks:
- id: ruff-check
args: [--fix]
files: ^(sentry_sdk/|tests/)
- id: ruff-format
files: ^(sentry_sdk/|tests/)
3 changes: 3 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,9 @@ ignore_missing_imports = true
# Target Python 3.7+ (minimum version supported by ruff)
target-version = "py37"

# Include files and directories
include = ["sentry_sdk/**/*.py", "tests/**/*.py"]

# Exclude files and directories
extend-exclude = [
"*_pb2.py", # Protocol Buffer files (covers all pb2 files including grpc_test_service_pb2.py)
Expand Down
3 changes: 1 addition & 2 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,5 @@ commands =

[testenv:linters]
commands =
ruff check tests sentry_sdk
ruff format --check tests sentry_sdk
pre-commit run --all-files
mypy sentry_sdk
Comment on lines +928 to 929
Copy link

@sentry sentry bot Dec 26, 2025

Choose a reason for hiding this comment

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

Bug: The ruff-format pre-commit hook is missing the --check argument, causing the linters tox environment to modify files in-place instead of only checking for formatting issues.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The linters tox environment was changed to use pre-commit run --all-files instead of a direct ruff format --check command. However, the ruff-format hook in .pre-commit-config.yaml is missing the args: [--check] configuration. As a result, running tox -e linters no longer just checks for formatting issues but actively reformats files in-place. This is a functional regression, as a linting environment is expected to be a read-only check, especially in CI, and should not modify source files.

💡 Suggested Fix

Add args: [--check] to the ruff-format hook in the .pre-commit-config.yaml file to restore the previous check-only behavior and prevent file modifications during linting.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: tox.ini#L928-L929

Potential issue: The `linters` tox environment was changed to use `pre-commit run
--all-files` instead of a direct `ruff format --check` command. However, the
`ruff-format` hook in `.pre-commit-config.yaml` is missing the `args: [--check]`
configuration. As a result, running `tox -e linters` no longer just checks for
formatting issues but actively reformats files in-place. This is a functional
regression, as a linting environment is expected to be a read-only check, especially in
CI, and should not modify source files.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7943798

Copy link
Author

@ulgens ulgens Dec 26, 2025

Choose a reason for hiding this comment

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

I believe this is okay. Explicitly saying that files shouldn't be changed doesn't yield a benefit on CI.