add pre-commit hooks using pre-commit

[homsar]

This addresses the suggestion in #237.

It uses pre-commit to automatically run the four sets of stylistic checks currently run in CI:

• black
• mypy
• flake8
• eslint

It also does a couple of extra checks that are in the default pre-commit config:

• Check for trailing whitespace on lines
• Check for blank lines at end of file
• Check that YAML syntax is valid
• Check that no large files have been added accidentally

It doesn't run the test suite, as the consensus seems to be that that takes too long to be a good commit hook, slowing down the development process. (This also avoids the issue of avoiding needing to squash whatever database was being developed against in favour of having the test database loaded, although I doubt this would be insurmountable.)

pre-commit automatically creates isolated environments in which to run all tests apart from mypy, which requires an active virtual environment with all requirements installed. (The alternative is maintaining a parallel copy of requirements.txt inside .pre-commit-config.yaml, which seems to introduce too high a maintenance burden. This is apparently due to problems with cache invalidation when reading requirements from any file other than .pre-commit-config.yaml.)

Also included are changes to a few files that had trailing whitespace that pre-commit picked up on first run. flake8 also complains about line length in some migrations, but I have not acted on this.

Two possible further improvements:

• Currently there is redundancy between the specification of rules in .pre-commit-config.yaml and in the combination of package.json and .github/workflows/test.yml. (In particular, the dependencies for eslint are specified twice.) This could be in principle rationalised, so that GitHub Actions uses pre-commit rather than having its own separate specification, and developers run pre-commit run rather than the current npm run-script.
• There could be a guide (CONTRIBUTING.md?) walking new contributors through setting up pre-commit (and any other setup steps, although most others are already covered in PANIC.md.

[colons]

i guess i have to do some reading here. if git has support for hooks, i am very confused about why it would be necessary to involve any software other than a single in-repo script to run the checks you're interested in. i'm especially confused about why it needs to maintain an independent dependencies list.

i also really don't like the idea of enforcing additional checks here that aren't checked for in CI.

[homsar]

i guess i have to do some reading here. if git has support for hooks, i am very confused about why it would be necessary to involve any software other than a single in-repo script to run the checks you're interested in

git’s own hook mechanism is clunky, and can’t be committed to the repo due to the way it lives in the .git directory. pre-commit automates the process of setting it up, and gives access to a library of standard hooks.

If you don’t have mypy or eslint installed? No problem, pre-commit will set them up for you.

the alternative would be a long docs page on how to install the various dev dependencies and add the relevant paths to the local copy’s pre-commit file. More to go wrong, more of a barrier to setting it up.

i'm especially confused about why it needs to maintain an independent dependencies list.

This is very fair imo. According to pre-commit’s author it’s because the virtual environments used are cached, with cache invalidation based on changes to the pre-commit config. Having dependencies in other files would (apparently) make cache invalidation an intractable problem so a long environment installation process would be needed on every commit, which is unacceptable to them (and probably to users too)

i also really don't like the idea of enforcing additional checks here that aren't checked for in CI.

Also very fair. If you decide to adopt pre-commit, then you can decide whether the extra checks are worth having or not, and harmonise with CI appropriately (with or without adopting pre-commit in CI)

[colons]

okay, yeah. it seems like this 'pre-commit' project is two things at once, and i don't like that

if these checks are worth enforcing (and, fwiw, i think they are; i'm mad about how much trailing whitespace you found), they should be enforced everywhere, and not just be checked if a dev setup is configured correctly. i would very much appreciate a pull request that did that.

i would prefer, though, to not introduce new tools to solve the problem described in #237. to me, something like d699ad0d76f834b5a865cc26abad74a3c6338177...c1ff0abd0d144a05c5ce71ade5e9b6cb5ce7d38a is plenty

[colons]

git’s own hook mechanism is clunky

hard disagree here, sorry. putting some executable scripts in a directory is just about the simplest possible way you could achieve this. maybe this is my plan9 appreciation showing, but i'd much rather have than than an obtuse configuration file with 'github.com' written in it. especially if the latter is doing the former anyway, but hiding it from me.

we can just have a 'hooks' directory that we encourage people to symlink from .git/hooks (or symlink its contents piecemeal) and be done with it. this is what i've done in d699ad0d76f834b5a865cc26abad74a3c6338177...c1ff0abd0d144a05c5ce71ade5e9b6cb5ce7d38a

If you don’t have mypy or eslint installed? No problem, pre-commit will set them up for you.

the alternative would be a long docs page on how to install the various dev dependencies and add the relevant paths to the local copy’s pre-commit file. More to go wrong, more of a barrier to setting it up.

this is huge, huge feature creep for a project called 'pre-commit'

if our developer setup documentation isn't good enough, new devs are going to have problems long before they get to a point where they have something worth committing. that's exclusively a problem with our documentation, and not something for a tool like this to solve. and if this thing maintains its own set of dependencies, that's not actually helping at all

[homsar]

hard disagree here, sorry. putting some executable scripts in a directory is just about the simplest possible way you could achieve this.

It's been a while since I tried to set up commit hooks from scratch; I remember some issues with needing absolute paths in some cases. I'm also probably doing a bad job of representing the aims of pre-commit, because I'm highlighting what's stood out to me about it rather than what its own aims are.

All I can say is that until I encountered a project using pre-commit, I never managed to find time to actually follow the steps to install pre-commit hooks and get them working, as it was an extra step on top of the environment setup that had already taken me too long. With pre-commit, that hasn't been the case, and I've been a much stronger proponent of adopting them. Maybe that's because I jump between projects a lot, or maybe it's because I am not a Plan 9 user—even though I think in most circumstances I lean towards shell-script based solutions over alternatives.

i would prefer, though, to not introduce new tools to solve the problem described in https://github.com/very-scary-scenario/nkd.su/issues/237. to me, something like https://github.com/very-scary-scenario/nkd.su/compare/d699ad0d76f834b5a865cc26abad74a3c6338177...c1ff0abd0d144a05c5ce71ade5e9b6cb5ce7d38a is plenty

Fair enough. One difference (I think, assuming I'm understanding git hooks correctly) is that this setup will check (and in the case of black, reformat) the complete repository, whereas pre-commit I think would only check files to be changed by the commit unless you told it otherwise.

if these checks are worth enforcing (and, fwiw, i think they are; i'm mad about how much trailing whitespace you found), they should be enforced everywhere, and not just be checked if a dev setup is configured correctly. i would very much appreciate a pull request that did that.

Cool, I'll try and do that this evening.

[colons]

i would be interested in knowing what you got stuck on. if it's what i think it is, that's entirely fair: i will absolutely concede that making relative symlinks from your shell is a minefield. the nature of <target> in the command ln -s <target> <symlink> is not at all what anyone would probably expect at first. my general rule at this point is to only ever make relative symlinks while cdd into the directory i want the symlink to live

pre-commit I think would only check files to be changed by the commit unless you told it otherwise.

nkd.su's codebase is small enough that i doubt this matters. for mypy in particular, this would be actively harmful, since you're as likely to break something in another file as your own, and i don't think mypy is actually at all quicker to check just one file than to check the whole repository, since it has to understand everything to type-check anything

if these checks are worth enforcing (and, fwiw, i think they are; i'm mad about how much trailing whitespace you found), they should be enforced everywhere, and not just be checked if a dev setup is configured correctly. i would very much appreciate a pull request that did that.

Cool, I'll try and do that this evening.

please do, i would very much appreciate it

[homsar]

i would be interested in knowing what you got stuck on. if it's what i think it is, that's entirely fair: i will absolutely concede that making relative symlinks from your shell is a minefield. the nature of in the command ln -s is not at all what anyone would probably expect at first. my general rule at this point is to only ever make relative symlinks while cdd into the directory i want the symlink to live

That might have been one issue, but not the only one I don't think. But it's been long enough that I don't remember what the actual issue was.

pre-commit I think would only check files to be changed by the commit unless you told it otherwise.

nkd.su's codebase is small enough that i doubt this matters. for mypy in particular, this would be actively harmful, since you're as likely to break something in another file as your own, and i don't think mypy is actually at all quicker to check just one file than to check the whole repository, since it has to understand everything to type-check anything

I'd need to check the details of the mypy pre-commit hook to know whether it causes that problem or not, but since we're not going with it I'll defer that for now.

But something that affects my workflow at least: if you have your virtualenv in the repository directory, then black gets very upset with the site-packages.

Edit: this may be my own damn fault for not using one of the standard venv directory names that black is configured to exclude.

Edit 2: but flake8 doesn't do this

[colons]

please yell at me if there's anything actionable for me here