Skip to content
Merged
26 changes: 18 additions & 8 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,31 @@ default_stages: [commit]
repos:
- repo: local
hooks:
- id: format
name: Format
# File-aware format hook - only runs on changed Python files
# Runs both ruff check --fix and ruff format
- id: format-files
name: Format Changed Files
stages: [commit]
language: system
entry: make format-python
pass_filenames: false
- id: lint
name: Lint
types: [python]
entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' --
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.

🟡 && in format-files pre-commit hook skips ruff format when unfixable lint violations exist

The format-files pre-commit hook entry uses && to chain ruff check --fix and ruff format. When ruff check --fix encounters lint violations that cannot be auto-fixed, it returns exit code 1, and the && operator prevents ruff format from executing. This means files with unfixable lint issues will never be auto-formatted.

Root Cause and Impact

The entry at .pre-commit-config.yaml:13 is:

entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' --

ruff check --fix returns exit code 1 when violations remain after applying all available auto-fixes (e.g., rules that have no auto-fix available). The && operator short-circuits, so ruff format is never invoked.

Notably, the corresponding format-python-files Makefile target at Makefile:64-65 correctly uses ; (semicolons) to separate the commands, ensuring ruff format always runs regardless of ruff check --fix's exit code:

		uv run ruff check --fix $(FILES); \
		uv run ruff format $(FILES); \

Impact: When a developer commits Python files that have unfixable lint violations (which is common — many ruff rules are not auto-fixable), the formatting step is silently skipped. The developer's files won't be auto-formatted, defeating the purpose of the format hook.

Suggested change
entry: bash -c 'uv run ruff check --fix "$@" && uv run ruff format "$@"' --
entry: bash -c 'uv run ruff check --fix "$@"; uv run ruff format "$@"' --
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

pass_filenames: true

# File-aware lint hook - only runs on changed Python files
# Runs both ruff check and ruff format --check
- id: lint-files
name: Lint Changed Files
stages: [commit]
language: system
entry: make lint-python
pass_filenames: false
types: [python]
entry: bash -c 'uv run ruff check "$@" && uv run ruff format --check "$@"' --
pass_filenames: true

# Conditional template hook - only runs when template files or roadmap change
- id: template
name: Build Templates
stages: [commit]
language: system
files: ^infra/templates/|\.jinja2$|^docs/roadmap\.md$
entry: make build-templates
pass_filenames: false
4 changes: 2 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ test-python-integration: ## Run Python integration tests (CI)
# Integration tests with better parallelization
test-python-integration-parallel: ## Run integration tests with enhanced parallelization
uv run python -m pytest sdk/python/tests/integration \
-n auto --dist loadscope \
-n auto --dist loadgroup \
--timeout=300 --tb=short -v \
--integration --color=yes --durations=20

Expand All @@ -209,7 +209,7 @@ test-python-integration-local: ## Run Python integration tests (local dev mode)
HADOOP_HOME=$$HOME/hadoop \
CLASSPATH="$$( $$HADOOP_HOME/bin/hadoop classpath --glob ):$$CLASSPATH" \
HADOOP_USER_NAME=root \
uv run python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \
uv run python -m pytest --tb=short -v -n auto --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \
-k "not test_lambda_materialization and not test_snowflake_materialization" \
-m "not rbac_remote_integration_test" \
--log-cli-level=INFO -s \
Expand Down
3 changes: 3 additions & 0 deletions sdk/python/pytest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ markers =
rbac_remote_integration_test: RBAC and remote functionality tests
integration: Integration tests (slower, requires services)
benchmark: Benchmark tests
slow: Tests taking >30 seconds
cloud: Tests requiring cloud credentials
local_only: Tests that run entirely locally

timeout = 300
timeout_method = thread
Expand Down
Loading