chore: add drf-lint pre-commit hook with baseline#5498
chore: add drf-lint pre-commit hook with baseline#5498blarghmatey wants to merge 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
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.
| - repo: https://github.com/mitodl/ol-django | ||
| rev: 93323f711f601cae27606ed50f3b8379a819c7b4 | ||
| hooks: | ||
| - id: drf-serializer-orm-check |
There was a problem hiding this comment.
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"]| - --exclude-files | ||
| - ".*/generated/" | ||
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||
| rev: "v0.0.287" |
| name: eslint | ||
| description: "Lint JS/TS files and apply automatic fixes" | ||
| entry: npx eslint --fix | ||
| language: node |
There was a problem hiding this comment.
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| entry: npx stylelint --allow-empty-input --fix $@ | ||
| language: node |
There was a problem hiding this comment.
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: systemd8f9c6d to
5ca0708
Compare
baf2582 to
e8ed78c
Compare
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>
e8ed78c to
ad13d9d
Compare
What are the relevant tickets?
N/A
Description (What does it do?)
Adds the
drf-serializer-orm-checkpre-commit hook from mitodl/ol-django which usesmitol-drf-lintto detect Django ORM queries inside DRF serializer methods — a common source of N+1 query bugs.Also generates an initial
drf_lint_baseline.jsonrecording all pre-existing violations so the hook only flags new issues going forward. Existing violations can be addressed incrementally.mitol-drf-lintis added as a dev dependency to support running the tool locally outside of pre-commit.How can this be tested?
pre-commit installserializers.pyfile that calls ORM methods (e.g..filter(),.get()) inside a serializer methoduv run drf-lint <path/to/serializers.py>Additional Context
The baseline was generated with: