Skip to content
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.

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

Precommit hook #278

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

homsar
Copy link
Contributor

@homsar homsar commented Sep 2, 2023

This supersedes #240 and addresses #237

Based on discussions on Slack I suspect this may not be mergeable, but wanted to discuss anyway.

My priorities when implementing this were roughly prioritised as:

  • The hook should never block a commit that would pass all of the checks in isolation (no false positives—it shouldn't matter if the working directory is dirty, or because the hook implementation doesn't function correctly)
  • The hook should prevent commits that do not pass any of the checks that can run in a single-digit number of seconds. There should be no need for placate flake8 commits. (git commit --no-verify is right there if it is needed, but my strong opinion is that hooks need to introduce friction to be meaningful, otherwise they are like compiler and deprecation warnings and will be ignored until they do cause friction)
  • Per discussion in add pre-commit hooks using pre-commit #240, avoid using .pre-commit-config.yaml
  • Per discussion on Slack, do not temporarily or otherwise destroy anything in the working directory.
  • Do not change anything in the commit (that should be a smudge/clean filter instead) or in the working directory (this is a difference from the .pre-commit-config.yaml way of doing things, because I usually get confused when I try to commit and it rewrites my stuff)

Things it checks:

  • Superfluous whitespace at EOL and EOF (by using git diff-index's built-in functionality)
  • Missing newlines at EOF (by doing some horrible parsing of git diff-index output)
  • black
  • flake8
  • mypy
  • Django check-migrations
  • building the documentation with make html
  • eslint

All of these apart from the first two require creating a copy of the repo in a temporary directory to be able to remove any files that the linters shouldn't check without affecting any open file handles in the working directory. (This also helps avoid polluting the reflog.) If creating that fails, these tests are skipped. The directory is removed before the hook finishes, including if it is aborted with Ctrl+C.

This requires a non-zero amount of machinery, but significantly less than used by .pre-commit-config.yaml, and is what I would consider the minimal required to satisfactorily run linters in a pre-commit hook (in a way I would be willing to use in my workflow).

Tachibana Kanade added 9 commits July 30, 2023 13:32
eslint is a requirement,
but doesn't appear to get added to the PATH by default.
using `npm exec` allows it to run without any additional setup.
this in fact generates multiple problems:
 - pollutes the reflog and causes git's automatic cleaning functionality to fail
 - breaks any active pipes to untracked or unstaged files
 - probably other issues too
switching from stash to clean meant that we weren't purging
changes not staged for commit, so these were being checked by linters
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