Skip to content

chore: add drf-lint pre-commit hook with baseline#5498

Open
blarghmatey wants to merge 1 commit intomasterfrom
chore/add-drf-lint-pre-commit
Open

chore: add drf-lint pre-commit hook with baseline#5498
blarghmatey wants to merge 1 commit intomasterfrom
chore/add-drf-lint-pre-commit

Conversation

@blarghmatey
Copy link
Copy Markdown
Member

What are the relevant tickets?

N/A

Description (What does it do?)

Adds the drf-serializer-orm-check pre-commit hook from mitodl/ol-django which uses mitol-drf-lint to detect Django ORM queries inside DRF serializer methods — a common source of N+1 query bugs.

Also generates an initial drf_lint_baseline.json recording all pre-existing violations so the hook only flags new issues going forward. Existing violations can be addressed incrementally.

mitol-drf-lint is added as a dev dependency to support running the tool locally outside of pre-commit.

How can this be tested?

  1. Install pre-commit hooks: pre-commit install
  2. Modify or create a serializers.py file that calls ORM methods (e.g. .filter(), .get()) inside a serializer method
  3. Attempt to commit — the hook should flag the new violation
  4. Alternatively, run directly: uv run drf-lint <path/to/serializers.py>

Additional Context

The baseline was generated with:

uv run drf-lint --generate-baseline --baseline drf_lint_baseline.json $(find . -name "serializers.py" ! -path "*/migrations/*")

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request establishes a pre-commit workflow by adding a configuration file with various linting hooks and integrating the mitol-drf-lint tool. The changes also include a baseline file for serializer violations and updated project dependencies. Review feedback identifies several necessary adjustments to the pre-commit hooks, including adding the baseline argument to the DRF linter, updating the ruff version, and switching the eslint and scss-lint hooks to use the system language for proper execution.

Comment thread .pre-commit-config.yaml
- repo: https://github.com/mitodl/ol-django
rev: 93323f711f601cae27606ed50f3b8379a819c7b4
hooks:
- id: drf-serializer-orm-check
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The drf-serializer-orm-check hook is missing the --baseline argument. Based on the PR description and the inclusion of drf_lint_baseline.json, this argument is required for the linter to ignore existing violations. Without it, the hook will report all pre-existing issues on every commit, defeating the purpose of the baseline.

      - id: drf-serializer-orm-check
        args: ["--baseline", "drf_lint_baseline.json"]

Comment thread .pre-commit-config.yaml
- --exclude-files
- ".*/generated/"
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: "v0.0.287"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The version of ruff specified (v0.0.287) is significantly outdated. It is recommended to update to a more recent version (e.g., v0.9.7) to benefit from numerous bug fixes, new linting rules, and performance improvements.

    rev: "v0.9.7"

Comment thread .pre-commit-config.yaml
name: eslint
description: "Lint JS/TS files and apply automatic fixes"
entry: npx eslint --fix
language: node
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The local eslint hook uses language: node with npx but does not define additional_dependencies. This configuration will likely fail in the pre-commit managed environment. If the intention is to use the tools installed in the project's local node_modules, language: system should be used instead.

        language: system

Comment thread .pre-commit-config.yaml
Comment on lines +93 to +94
entry: npx stylelint --allow-empty-input --fix $@
language: node
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For the scss-lint hook, the $@ placeholder is unnecessary as pre-commit automatically appends the file list to the command. Additionally, similar to the eslint hook, language: system should be used if you intend to rely on the local node_modules via npx.

        entry: npx stylelint --allow-empty-input --fix
        language: system

@blarghmatey blarghmatey force-pushed the chore/add-drf-lint-pre-commit branch 2 times, most recently from d8f9c6d to 5ca0708 Compare April 1, 2026 20:14
Comment thread .pre-commit-config.yaml
@blarghmatey blarghmatey force-pushed the chore/add-drf-lint-pre-commit branch 4 times, most recently from baf2582 to e8ed78c Compare April 1, 2026 20:50
Adds the drf-serializer-orm-check hook (mirroring the definition from
mitodl/ol-django) to detect Django ORM queries inside DRF serializer
methods (N+1 risk). Uses repo: local so pre-commit only installs
mitol-drf-lint without attempting to pip-install the ol-django workspace
root (which has internal workspace dependencies unavailable on PyPI).

Generates an initial baseline file (drf_lint_baseline.json) recording
existing violations so the hook only flags new issues going forward.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@blarghmatey blarghmatey force-pushed the chore/add-drf-lint-pre-commit branch from e8ed78c to ad13d9d Compare April 2, 2026 19:43
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.

1 participant