MNT Add pre-comit configuration #16957
Conversation
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
|
Thanks @NicolasHug I addressed your comments.
Nowhere bad, just looking for reviewers who could approve this PR ^^ |
| - repo: https://gitlab.com/pycqa/flake8 | ||
| rev: 3.7.8 | ||
| hooks: | ||
| - id: flake8 |
There was a problem hiding this comment.
Does this follow the configuration in examples/.flake8 when linting examples?
There was a problem hiding this comment.
Well right now this only checks for one single error (F401: unused imports) which is also compatible with examples/.flake8. Now it's more a proof of concept for flake8, I'd be happy extend it once we have this pre-commit config file merged :)
| $ git add modified_files | ||
| $ git commit | ||
| $ pip install pre-commit | ||
| $ pre-commit install |
There was a problem hiding this comment.
I think you might need to mention that occasionally (particularly when creating merge commits), you might need to use git commit -n to disable pre-commits.
There was a problem hiding this comment.
What error message will users get the first time?
If it's explicit enough so that a simple google search give them the answer, I'd leave it out.
This section is aimed at new contributors and we need to be careful about not overloading with information (that was an issue in the previous version of the guide that we tried to trim down). Also I honestly doubt they'll ever remember "oh right I saw that in the UG"
They only run on files that changed, but on the full file not just the diff.
Well it's exactly the same error message they would get in CI: i.e. the error message from from flake8 or mypy saying that at Line N something is wrong. Ideally we should aim for the situation where the same linters are run in pre-commit and in CI. So it they disable pre-commit it would just later fail in CI and still need fixing. For the currently added checks I don't think there is a use case where where one would need to disable pre-commit (worst case scenario is adding |
Actually, you are right for merge commits. Ignore my earlier message. Added the sentence to the doc, it cannot hurt. |
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
This adds a pre-commit setup to run a number of code style checks locally before each commit.
This is fully op-in, and only affect contributors who install pre-commit locally. Most of the below checks are already run in CI, so this allows to fail earlier. Among other things this would help not overwhelming CI during sprints.
It runs only on modified files and currently,
When one does
pre-commit installthis install all of the above into a virtualenv, so in the long run it can simplify our contributing guide by not asking to install them manually. It also pins versions of these tools to avoid a situation where locally linters pass but fail in CI.So far this doesn't add
pre-comitin CI, and pre-commit remains fully optional for contributors. Personally I would would like to be able to use it though, and it makes sense to share the config.For instance pandas has an extensive setup https://github.com/pandas-dev/pandas/blob/master/.pre-commit-config.yaml
cc @thomasjpfan @NicolasHug @adrinjalali