Contributing pre-commit hooks

[brucellino]

Use the pre-commit framework

I have seen a few issues and PRs raised about code quality, formatting, etc., e.g. #304

I think adding a pre-commit configuration could help.

Summary of proposed changes

I usually avoid this problem by using pre-commit hooks. by the time you've pushed the code, it's too late and if the code is formatted in the pipeline, it has to make a new commit -- generally a bit messy. Better to insist on clean commits from the author in the first place. Unfortunately, the only way to really enforce this is with the pre-receive hooks, and those are only available on the enterprise plan.

But pre-commit hooks can be easily slotted into the pipeline, and they can be used to break the build if they exit != 0

The pre-commit hook in particular is https://github.com/pre-commit/mirrors-prettier

Would that be an idea you wish to explore? If so, I'd be happy to make my first PR 🙂

[gwarf]

So it's about documenting how users could set those .pre-commit hooks into their local repo fork+clone? How does it work with PR that are made by creating a branch in this upstream repo? We also don't want to just reject any submission (like from less technical people using only the basic github web UI to make some small change), at least once we have a PR we can help getting things cleaned.

[brucellino]

Yes, the .pre-commit-config.yml file declares which hooks are active.

We can reasonably expect a contributor to install them in their development environment:

$ pip install pre-commit
$ pre-commit install

It is not required to install them, but since they declare the baseline hygiene of the codebase, it makes it much easier to converge on. Contributors can look at them and have an idea of the conventions adopted in the codebase.

They can be included in all PR CI runs either breaking the build, or emitting a warning.

As I mentioned, running pre commit hooks after the actual commit has happened is a bit ... "wrong", so the best case scenario is that they are installed and used by the main contributors. If not, you run the risk of having to manually clean the commit history if something bad (like a secret, a huge file, or something that breaks prod) gets into the codebase by accident.

Shall I send a PR and we can have a look?

[brucellino]

WIP #636

[brucellino]

This can be closed -- sorry forgot to mention it in #636

[brucellino]

Closed by #636