Add pre-commit hook config to run flake8 and black on commit

[austinweisgrau]

Notes added to README on how to install and set up

Resolves #863

[Jason94]

@austinweisgrau Will this actually reformat the code in the commit? Or will it just block the PR if it fails to pass the formatter?

[austinweisgrau]

It will reformat and block the commit if anything doesn't line up. The changes need to be manually staged. If there are no changes needed, the commit goes through.

[Jason94]

It will reformat and block the commit if anything doesn't line up. The changes need to be manually staged. If there are no changes needed, the commit goes through.

I see. Not going to lie, I thought we already enforced flake8 and black formatting. Maybe that's just on PR, not on commit? @shaunagm? Either way, thanks for setting this up!

[austinweisgrau]

We enforce it for a PR to be able to merge, but otherwise it's just part of the suite of CI tests, so it's possible to commit and push code that is in violation. Pre-commit hooks are a tool that enforce earlier on, which can be helpful. Similar to setting up lint-on-save in an IDE, it's just another layer to help developers out. If developers don't install pre-commit, it won't be in effect for them. So it's sort of ultimately up to the individual developer, unlike linting in the CI layer.

On Mon, Aug 7, 2023 at 12:12 PM Jason @.***> wrote:

It will reformat and block the commit if anything doesn't line up. The changes need to be manually staged. If there are no changes needed, the commit goes through.

I see. Not going to lie, I thought we already enforced flake8 and black formatting. Maybe that's just on PR, not on commit? @shaunagm https://github.com/shaunagm? Either way, thanks for setting this up!

— Reply to this email directly, view it on GitHub https://github.com/move-coop/parsons/pull/864#issuecomment-1668438973, or unsubscribe https://github.com/notifications/unsubscribe-auth/AO74QHWKEICK6PQT6VF32JDXUE4YLANCNFSM6AAAAAA3HLKM2Y . You are receiving this because you were mentioned.Message ID: @.***>

[shaunagm]

@austinweisgrau I've never personally used pre-commit hooks, what would the experience be like for someone who has them set up?

[austinweisgrau]

Pre-commit can be a little annoying if it has slight disagreements with the linter in the IDE or CI. It shouldn't really happen and should theoretically be simple to line up configurations to prevent it, but I've ran into some issues with this at times.

[austinweisgrau]

The experience is:

Setup:

• Run pip install pre-commit and pre-commit install

Thereafter: When you run git commit, pre-commit will run black and flake8 on your staged code at that time and you won't be able to commit if anything doesn't line up. Fixes will be automatically made for black so you can run git add . && git commit ... at that time. I believe you need to make the changes yourself for flake8 issues. If you have linting run automatically on save in your IDE, this should never come up and the only difference you'll notice is that git takes an extra second or so to run git commit then otherwise.

[austinweisgrau]

Personally I'm sort of neutral-positive on whether pre-commit is useful to add, but I had a setup ready and saw the issue come in so I figured I'd submit the PR. It's nice to have extra layers to help keep code linted, but with linting in CI this is pretty much taken care of at the repo-level. And I figure most people who are going to pay enough attention to install and set up pre-commit probably are also inclined to set up linting on save in their IDE, so it's potentially a little redundant.

[crayolakat]

I created the issue because I saw several people fail the automated tests when creating a PR because of linting/formatting issues, then having to go back and change fix it. Examples:

• https://github.com/move-coop/parsons/pull/859
• https://github.com/move-coop/parsons/pull/834

I think having this pre-commit would help prevent the extra step of having to go back and fix something after submitting a PR

[Jason94]

That makes sense to me. Might as well give people as much heads up as possible. It could also save compute time running the CI/CD.

[shaunagm]

Austin, would you be willing to write all this up as a very short guide for the website that we can link people to? It doesn't have to be much fancier than what you've currently written but it would be good to have it in one place we can link people to. I think many of the people who might be interested in using this will not have used git pre-commit hooks before and could use a little extra handholding (myself included).

Alternatively I can write it up and you can review and make sure I haven't garbled anything, but I wouldn't be able to get to that until next week at the earliest.

[austinweisgrau]

How's this? https://docs.google.com/document/d/186a8JB4q4fOZYYJoZV5EntWd9M4bEKg1WCREC4ydHCI/edit

[shaunagm]

Looks great! I'd like someone to test this locally and confirm it works before we merge. @crayolakat and @Jason94 I believe you both have permissions to do so, if you want to test and merge. If you're busy, no worries, I can do this myself next week.

[crayolakat]

I can test!

[crayolakat]

I tested and the instructions are very clear to easy to follow. Thank you Austin!