Skip to content

Conversation

@rodrigobnogueira
Copy link

@rodrigobnogueira rodrigobnogueira commented Dec 31, 2025

Improves the pre-commit configuration to provide local feedback that matches CI, reducing the need for multiple PR iterations due to linting issues.

Changes

Pre-commit Configuration (.pre-commit-config.yaml)

New Hook Structure:

  • Auto-fix hooks (run first on changed files):

    • lint: Auto-fixes import sorting and linting issues via ruff
    • format: Auto-fixes code formatting via ruff
  • Validation hooks (match CI exactly):

    • check-lint-strict: Mirrors just check-lint from CI
    • check-format-strict: Mirrors just check-format from CI
    • check-package: Mirrors just check-package (dependency check)
    • check-readme: Mirrors just check-readme (README rendering)
  • Manual hooks:

    • run-tests: Available for manual test execution

Key Improvements:

  • Direct uv run ruff commands instead of just delegation
  • pass_filenames: true for faster incremental checks
  • types: [python] filtering for efficiency
  • verbose: true for clearer development feedback

Ruff Configuration (pyproject.toml)

  • Added force-exclude = true to ensure exclusions work with pass_filenames: true
  • Added [tool.ruff.format].exclude for migration file exclusions

Why This Matters

Before this change, developers would push code, wait for CI, discover lint failures, fix them, push again, and repeat. This created a slow feedback loop.

Now, the pre-commit hooks:

  1. Auto-fix common issues before commit
  2. Validate that code will pass CI checks locally
  3. Properly exclude migration files
  4. Provide verbose, helpful output

Testing

All hooks pass on the full project:

Lint (auto-fix)...............Passed
Format (auto-fix).............Passed
Check Lint (CI match).........Passed
Check Format (CI match).......Passed
Check Package (CI match)......Passed
Check README (CI match).......Passed

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances the pre-commit configuration to better match CI validation commands, aiming to provide developers with immediate local feedback and reduce CI iteration cycles. The changes introduce a two-tier hook structure with auto-fix hooks followed by strict validation hooks that mirror CI checks.

Key Changes:

  • Restructured pre-commit hooks into auto-fix and validation categories with explicit CI command matching
  • Added force-exclude = true to ruff configuration and explicit format exclusions for migration files
  • Replaced just command delegation with direct uv run ruff commands for better control and performance

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
.pre-commit-config.yaml Expands from 2 hooks to 6 hooks with auto-fix (lint, format) and validation (check-lint-strict, check-format-strict, check-package, check-readme) stages, plus an optional manual test hook
pyproject.toml Adds force-exclude = true to ruff configuration and explicit format exclusions to ensure migration files are properly excluded when using pass_filenames: true

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# Global checks (must run on whole project/env)
- id: check-package
name: Check Package (CI match)
entry: uv pip check
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The command 'uv pip check' differs from the CI command which uses 'uv run pip check' (via 'just run pip check' in justfile line 62). The pre-commit hook should use 'uv run pip check' to match CI behavior exactly.

Suggested change
entry: uv pip check
entry: uv run pip check

Copilot uses AI. Check for mistakes.

- id: check-readme
name: Check README (CI match)
entry: uv run --group docs python -m readme_renderer ./README.md -o /tmp/README.html
Copy link

Copilot AI Dec 31, 2025

Choose a reason for hiding this comment

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

The command syntax differs from CI. The justfile (line 154) uses 'uv run -m readme_renderer ./README.md -o /tmp/README.html' while this uses 'uv run --group docs python -m readme_renderer'. To match CI exactly, this should be 'uv run -m readme_renderer ./README.md -o /tmp/README.html'. The '--group docs' flag and 'python -m' pattern are unnecessary since CI installs docs dependencies beforehand and 'uv run -m' directly invokes the module.

Suggested change
entry: uv run --group docs python -m readme_renderer ./README.md -o /tmp/README.html
entry: uv run -m readme_renderer ./README.md -o /tmp/README.html

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

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

done

@JohananOppongAmoateng
Copy link
Contributor

i dont know if its a good idea and would be great but i think it would be nice to add running specific test files that change for each commit.

@codecov
Copy link

codecov bot commented Dec 31, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.91%. Comparing base (139db51) to head (4e5f591).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #779   +/-   ##
=======================================
  Coverage   82.91%   82.91%           
=======================================
  Files          26       26           
  Lines        1580     1580           
  Branches      250      250           
=======================================
  Hits         1310     1310           
  Misses        206      206           
  Partials       64       64           

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This PR significantly improves the pre-commit configuration to provide
local feedback that matches CI, reducing the need for multiple PR
iterations due to linting issues.

## Changes

### Pre-commit Configuration (.pre-commit-config.yaml)

**New Hook Structure:**
- **Auto-fix hooks** (run first on changed files):
  - `lint`: Auto-fixes import sorting and linting issues via ruff
  - `format`: Auto-fixes code formatting via ruff

- **Validation hooks** (match CI exactly):
  - `check-lint-strict`: Mirrors `just check-lint` from CI
  - `check-format-strict`: Mirrors `just check-format` from CI
  - `check-package`: Mirrors `just check-package` (dependency check)
  - `check-readme`: Mirrors `just check-readme` (README rendering)

- **Manual hooks:**
  - `run-tests`: Available for manual test execution

**Key Improvements:**
- Direct `uv run ruff` commands instead of `just` delegation
- `pass_filenames: true` for faster incremental checks
- `types: [python]` filtering for efficiency
- `verbose: true` for clearer development feedback
- Removed placeholder `check-types` hook (mypy not configured)

### Ruff Configuration (pyproject.toml)

- Added `force-exclude = true` to ensure exclusions work with
  `pass_filenames: true`
- Added `[tool.ruff.format].exclude` for migration file exclusions

## Why This Matters

Before this change, developers would push code, wait for CI, discover
lint failures, fix them, push again, and repeat. This created a slow
feedback loop (experienced in PR jazzband#391).

Now, the pre-commit hooks:
1. Auto-fix common issues before commit
2. Validate that code will pass CI checks locally
3. Properly exclude migration files
4. Provide verbose, helpful output

## Testing

All hooks pass on the full project:
```
Lint (auto-fix)...............Passed
Format (auto-fix).............Passed
Check Lint (CI match).........Passed
Check Format (CI match).......Passed
Check Package (CI match)......Passed
Check README (CI match).......Passed
```
@rodrigobnogueira
Copy link
Author

Thanks for the feedback!

@copilot suggestions:

  • Removed redundant **/migrations/*.py pattern
  • Simplified check-readme to use uv run -m readme_renderer
  • Kept uv pip check instead of uv run pip check - the latter fails because pip is a uv subcommand, not a Python script. Note: this means the justfile's check-package task may also have this issue.

@JohananOppongAmoateng Great idea about running specific tests for changed files. Never done that before.

@JohananOppongAmoateng
Copy link
Contributor

Thanks for the feedback!

@copilot suggestions:

  • Removed redundant **/migrations/*.py pattern
  • Simplified check-readme to use uv run -m readme_renderer
  • Kept uv pip check instead of uv run pip check - the latter fails because pip is a uv subcommand, not a Python script. Note: this means the justfile's check-package task may also have this issue.

@JohananOppongAmoateng Great idea about running specific tests for changed files. Never done that before.

Just something that came into my mind i dont know if its even possible but i was thinking if we make a change in file a we most likely would add new tests or remove tests for that file so i expect to see some tests files changing that could be run just for those. I am mean it just an idea dont know how feasible it is and whether its worth it

@bckohan
Copy link
Collaborator

bckohan commented Dec 31, 2025

i dont know if its a good idea and would be great but i think it would be nice to add running specific test files that change for each commit.

This would be nice but I'd like to keep commits fast and low barrier so its easy for people to share code. There are many cases where you might want to commit a failing test (WIP PRs or to demonstrate an issue in a branch).

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.

3 participants