Using pre-commit framework

[ccamara]

I've learnt the hard way that bookwyrm follows a style guide and has some tests that need to be passed before merging a PR.

This PR is an attempt to install the Pre-commit framework, which should implement those tests BEFORE creating a commit. While it may seem redundant, they are not:

• CI tests help to notice any error
• Pre-commit prevents errors from being pushed (this is, provided pre-commit is installed in the environment)

IMHO, having something like pre-commit would speed up the contributing process, as contributors (even new ones like me who would forget about manually running the tests) would immediately know if their code adheres to the style guide.

This is a draft. I can see, at least, the following items that should be addressed before merging:

• Find a way to install pre-commit in the environment automatically and/or update setup instructions (I updated requirements.txt but I'm not sure how the environment setup works in this project, as I'm not familiar with docker -also, not sure if we should be using requirements-dev.txt -feel free to suggest solutions)
• Decide what to include in pre-commit, i.e. (but not limited to):
  • curlylint
  • stylelint
  • ESLint
  • ...

Disclaimer: I've never used pre-commit before. Input is welcome.

This would fix #3213

[mouse-reeve]

Is the goal of this to run the checks locally before a commit is created? I have tried that and found that it makes it terribly slow to get anything done (especially as someone who tries to make many small commits), so I wonder if there's a different way to make those checks less of an unpleasant surprise for contributors?

(If I'm misunderstanding what this would do please let me know as well!)

[dato]

Hello all!

Is the goal of this to run the checks locally before a commit is created?

I understand pre-commit is often used prior to commit, though I find it less annoying running checks before push only.

so I wonder if there's a different way to make those checks less [unpleasant]

We could start with just a very fast check (e.g., black), and only on push (rather than over every commit). I would certainly welcome it, since I'm always forgetting to run black on migration files!

[…] less of an unpleasant surprise for contributors?

In this first stage, we could also mention in the contributing guide that the use is optional? (To clarify: this is the default, since nothing will run on developer machines unless they explicitly run pre-commit install first.)

@ccamara Would this make sense to you as an initial step?

P.S.: I haven't had time to check, but perhaps curlylint/stylelint/eslint are equally fast and could be included (plus, as far as I know they would only run if the push touches relevant files, i.e. non-python). From this PR, pylint is for sure slow, but could be configured to be run manually only.

[mouse-reeve]

Thank you for confirming! Even running black is too slow for me on a commit-by-commit basis, but pre-push sounds like a good compromise. And like you say, if it's opt-in, then it's a non-issue for people like me who have spent their lives on geriatric laptops where running simple commands is a get-up-and-make-tea level time commitment.

With the exception of black and eslint, I haven't used the other linters to actually fix code issues, and I'm not sure if they'd all be a good fit? But having Black by itself would be a big help, since it's practically impossible (at least for me) to write code exactly how it wants it :)

[ccamara]

Hello all!

Thanks for your input, and apologies for my late reply (life happens).

I need to make a disclaimer: this is the first time I've ever used pre-commit framework, and I'm now speaking from memory after not haven't used it for some months, so my responses may not be 100% correct.

@dato what you've described makes sense, but I'm afraid that's not how pre-commit works and I'm not even sure that it could be achieved using it (it seems to me that to do that, we'd need to use regular git hooks, which are a different approach -albeit with some overlaps).

What pre-commit does is basically run some "hooks" (not regular git hooks, hence the confusion) that are listed in a yaml file called .pre-commit-config.yaml (in this case, this one: https://github.com/ccamara/bookwyrm/blob/pre_commit/.pre-commit-config.yaml). These hooks can be to run linters or even custom scripts, and are usually maintained elsewhere (see all the available hooks here). Those hooks are run every time a commit is created (not before pushing, after a commit is created) and only for the changed files (although it is also possible to manually extend checks to all the files -see here). That means that:

• If everything is "as it should" (according to the hooks), the commit is created and nothing is changed.
• If there's something that is not as it should, a warning will be displayed, more or less in the same way as PR checks work or will be automatically fixed. Either way, it won't be possible to create the commit until the offending error is fixed or the hook is temporarily disabled -see here).

I recognise that not being able to create commits, or even seen some automated changes, might be frustrating for some, but IMHO, it has some advantages (besides what I described at #3213):

• It is not possible to push commits that do not adhere to the guidelines. That means that all commits are good and also getting rid of commits that are only stylistic.
• the PR checks could use the same hooks, meaning that there's only one thing to maintain (also, most of the hooks are community maintained, so we offload that from a relatively reduced and overloaded team at bookwyrm).
  • If we need to reduce the time spent on testing, we could potentially make PR tests to be run before merging, vs on every push, just as a precautionary measure, because pre-commit would have prevented all the issues beforehand (unless a committer decides to manually override it)
• these hooks could also make (some/all) the tests .bw-dev redundant, again, reducing the maintenance costs of this project.

I honestly haven't experienced any slowdown because of pre-commit. This could be because my laptop is fast, but I believe it is mainly because tests are only run for a subset of the modified files (based on the configuration on the yaml file, which lists the file extensions for the files to be checked for every hook, independently). Of course, it could be both, or that I'm wrong (not trying to gaslight you, @mouse-reeve !)

I hope this clarifies your questions/concerns (or at least some) to inform a decision.

[dato]

Hi @ccamara

@dato what you've described makes sense, but I'm afraid that's not how pre-commit works and I'm not even sure that it could be achieved using it (it seems to me that to do that, we'd need to use regular git hooks, which are a different approach -albeit with some overlaps).

Ah, but the pre-commit software uses plain git hooks under the hood. :)

By default, the ‘pre-commit’ Git hook is used (hence the name of the “pre-commit” software); but it can be configured to use other hooks (which pre-commit calls “hook types”).

For example, to use black before push we can set default_install_hook_type to only “pre-push”. See the following transcript, where “pre-commit install” only touches .git/hooks/pre-push, and a commit with single quotes is performed OK, and stopped by black only when trying to push:

❯ git show HEAD:./.pre-commit-config.yaml
default_install_hook_types:
  - pre-push  # default for BookWyrm
repos:
  - repo: https://github.com/psf/black
    rev: 'refs/tags/22.12.0:refs/tags/22.12.0'  # psf/black#2493
    hooks:
      - id: black
        types: [python]

❯ pre-commit install
pre-commit installed at .git/hooks/pre-push

❯ echo "'hi'" >bookwyrm/__init__.py && git commit -a -m 'commit is ok'
[pre-commit 827f59e98] commit is ok
 1 file changed, 1 insertion(+)

❯ git push

black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted bookwyrm/__init__.py

All done! ✨ 🍰 ✨
1 file reformatted.

error: failed to push some refs to 'https://github.com/dato/bookwyrm'

❯ git status -s
 M bookwyrm/__init__.py

[dato]

Furthermore, we’re only discussing the default for the repo.

If you or any other contributor prefers the default ‘pre-commit’ behavior, you still can install the hook manually:

❯ pre-commit install -t pre-commit
pre-commit installed at .git/hooks/pre-commit

❯ echo "'hi'" >bookwyrm/__init__.py && git commit -a -m 'commit is not ok'
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted bookwyrm/__init__.py

All done! ✨ 🍰 ✨
1 file reformatted.

[ccamara]

I see. Thanks for that. There's so much I didn't know about pre-commit!

[mouse-reeve]

When I installed this pre-commit, I kept getting:

Oh no! 💥 💔 💥 The required version `22` does not match the running version `0.1.dev1+g27d2014`!

I changed the version number in the .pre-commit file to 'refs/tags/22.3.0:refs/tags/22.3.0' based on reading psf/black#2493 and that got it working for me.