Proposal to add pre-commit

[smi...@gmail.com]

Hi All

I have opened a ticket[1]  which proposes to add pre-commit[2] to Django, and wish to gather thoughts on it here. 

For anyone not familiar pre-commit checks for simple issues before each `git commit`. The aim is to try and catch format errors closer to the developer and therefore reduce C/I tests that need to be re-run. 

I propose that we add pre-commit hooks for the existing code style checks (flake8, isort) and to check for the whitespace rule. In addition, we can add a hook for an empty last line. My thought on an initial setup is something like this[3].

Documentation would be added as part of the guide to getting set up with the Django test suite [4] and would include notes on how to install the pre-commit hooks. I also think a smaller note with the patch review checklist could also be useful.

Looking ahead I think this tool would be very useful to have in place for when Black (DEP8) is added as it will be much more strict on code style. This will result

in far more pull requests being opened and commits being pushed which will fail the code style checks quickly. If we can reduce the number of times this happens

then I think this is a good change. 

Final note is that while thinking about how to flag this to users I wondered if a pull request template would be helpful, with the checklist in it, and a notice about pre-commit? 

Kind Regards

David

[1] - https://code.djangoproject.com/ticket/32165

[2] - https://pre-commit.com/

[3] - https://gist.github.com/smithdc1/66e6f696beec25e47fdbbef4bca4ab34

[4] - https://docs.djangoproject.com/en/3.1/intro/contributing/#previewing-your-changes

[Adam Johnson]

Hi David

I like pre-commit too. I think it would be a great addition. Your initial setup looks good - and perhaps we can add a few more hooks from pre-commit-hooks like check-case-conflict and check-json.

One thing I prefer to do is use pre-commit to run binaries through "system" (e.g. [1]). This allows pinning through requirements.txt, reduces the number of virtualenvs, and reduces the number of places versions can be pinned. I'm open on which way we do this, but thought I'd share my solution.

Final note is that while thinking about how to flag this to users I wondered if a pull request template would be helpful, with the checklist in it, and a notice about pre-commit?

If pre-commit runs on CI this may just be noise. But open to see it - we don't have any PR template at the moment, and it could be helpful to point people to Trac and the contribution guide at least.

[1]:https://github.com/adamchainz/apig-wsgi/blob/master/.pre-commit-config.yaml

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/c114c95a-e9a4-4b25-8a6b-54ae4bac9b22n%40googlegroups.com.

--

Adam

[Carlton Gibson]

I’d be +1 here. I’ve found pre-commit very useful on other projects. 

[smi...@gmail.com]

Hi All,

Thanks for your comments. 

Adam -- yes, I quite like pre-commit-hooks it reminds me of the 'pick and mix' when I was a kid. We can easily add more here, as I guess the risk is low. 

I like the idea of running the binaries through the system but I wonder if it's too much here. That is, the *introduction* to the contributing guide doesn't talk about tox. So we'd need to change that guide to run via tox, for pre-commit to work?

As the comments here so far have been positive, I'll add it to my list to write a Pull Request for this.

David