Add pre-commit and run auto-formatters #699
Conversation
|
Downstream test failure is unrelated |
|
+1 , I trust you if you think precommit will help. I prefer merge commit
anyway as many things get screwed up by squash and merge and co, like
finding all merged branches.
…On Thu, Apr 7, 2022 at 10:47 AM Steven Silvester ***@***.***> wrote:
@blink1073 <https://github.com/blink1073> requested your review on: #699
<#699> Add pre-commit and run
auto-formatters.
—
Reply to this email directly, view it on GitHub
<#699 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACR5T2AOLKGGB5GLHTLORLVD2OLVANCNFSM5SYTFSXA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Could you update the development installation instructions to explain how to set up pre-commit?
I've also found that people can sometimes find pre-commit frustrating when hooks cannot automatically fix the issues they discover. For example, black works great as a pre-commit step for most people because it automatically fixes any stylistic mistakes, but flake8 can be annoying because it reports the error without fixing it and will block you from committing until you fix everything it's complaining about. If the commit is really just a WIP and you don't want to fix everything right then and there, you have to know to go look at the pre-commit docs and figure out how to turn off its checks for that commit.
Pre-commit checks like flake8 are fine to run in CI, but maybe not by default on ever commit a developer might make as they're writing code. Given this, it looks like pre-commit allows you to configure when hooks will run using stages. Perhaps for those hooks which do not automatically correct mistakes they could be marked as manual and only run in CI.
| # - repo: https://github.com/pre-commit/mirrors-mypy | ||
| # rev: v0.942 | ||
| # hooks: | ||
| # - id: mypy |
There was a problem hiding this comment.
I think this can be removed
There was a problem hiding this comment.
I am adding this in a follow up PR as well
|
Shoot, didn't see this was already merged. Maybe these minor nits could be done in a follow up? |
|
I'll follow up with docs and moving some of the checks to ci-only by using the manual stage. |
|
Awesome! |
|
Great feedback by the way @rmorshea, I'll use that approach across repos. |
This should be merged as a merge commit so git-blame-ignore-revs works.