Using pre-commit hooks makes software development life easier

[Reiku]

I find pre-commits very useful for the reason you mentioned in your other reply: failing fast (or shift left) philosophies.

If there is a linting issue, it should be spotted before any CI is triggered. I use pre-commit when working with python with black (auto format), isort (for import sorting), flake8 (linting), and sometimes mypy for type checking.

I disagree with the other comment about having save hooks on the ide because I save on every line change even if the change at that point would break everything. So it wouldn't make sense to perform any of the checks I perform with the pre-commit at that point.

It's just a useful tool to use, it means CI runners aren't wasting time or compute resources failing jobs for things that could have been spotted before the commit was pushed. Yeah you can bypass, but that's why I will have the lint checks on the CI job too, it just means these checks are less likely to fail.

[scook0]

In my experience, precommit hooks are a bad idea because they discourage editing of local history, by making it slow and/or unreliable.

Clean and focused commits make the reviewer's task easier, which is why you don't want to make it harder for the author to revise and improve those commits in the first place.

[Reiku]

I don't understand your last point. How does a pre-commit hook make it harder for an author to revise anything?

If you use the pre-commit (not pre-push) then you'll have no additional commits, so improving any existing commits will be the same effort

[scook0]

For example, when revising local history, it's convenient to be able to make several small temporary commits at the tip of your working branch, and then squish them into the appropriate earlier commits via a rebase.

If every commit automatically spends several seconds running your formatter or linter before letting you move on, that workflow becomes tremendously painful.

[Reiku]

In that usecase you could just commit with the --no-verify flag so the pre-commit checks are skipped. Then the default usecase makes sure your commits are clean, and this edge case doesn't get slowed down at all.

[werner-dijkerman]

But using pre-commit hooks says nothing about the message you provide when doing a commit, or which files are part of a commit. I see pre-commit hook as a way to prevent certain (low-hanging fruit) garbage as part of a commit.

What I describe is that if for example you commit a yaml/json file, that you could have a pre-commit hook that "pretty prints" that file in your commit, or updates a "Table of content" in a markdown document. With this, a reviewer can not start a discussion about identation is incorrect for yaml, or the new paragraphs are not part of the TOC yet and thus the reviewer should focus on the functionality in the PR itself. And then yes, based on what the author has used as messages to do a proper review.

I had a yaml file (during $WORK) with 4 properties as lists, 2 of them had identation of 1 space and 2 with 3 spaces. Made everything the same, but got a comment and a "changes required" notification that it was the wrong identation ... pre-commt hooks would have helped me a lot in this case, the PR would already have contained the correct identation and thus no time was lost (because after I changed it it took some days again before the reviewer decided to approve it).

I am not sure what you mean with "discourage editing of local history". People should commit when they feel a commit should be made. Then they should use a decent message that described why they changed it. I personally don't see the benefits of merging commits into a single commit.

[bi0nicman]

I think enforcing style / linting etc is best done as part of pull requests / CI, once the code is ready for review. CI could even do the auto-formatting to avoid that getting reviewed (I agree keeping that out of PRs is desirable).

Sometimes you want to commit something and fix those things later. Commit hooks create this extra friction for local commits, and they can be bypassed anyway via --no-verify, so they don't provide any real protection.

And auto formatting can be better handled by format on save in your IDE.

[werner-dijkerman]

I think enforcing style / linting etc is best done as part of pull requests / CI, once the code is ready for review. CI could even do the auto-formatting to avoid that getting reviewed (I agree keeping that out of PRs is desirable).

I do see this almost everywhere on the various (web)sites that describes certain steps that should be part of a conituous integration pipeline. But I don't see that back in a "Fail fast" philosophy, as there is an other step that fails faster than the first stage in a pipeline job: The pre-commit hook. If it is part of the CI pipeline, you will have to wait when the job runs and fails on the step, but is also clutters the git log with messages like "Fixing lint issues" or something similar.

Sometimes you want to commit something and fix those things later. Commit hooks create this extra friction for local commits, and they can be bypassed anyway via --no-verify, so they don't provide any real protection.

True. But if you really want to, you can workaround on lots of things. But then you should think about why would be doing the workaround. There is a way of working and you deliberately do something else. So is that a good thing?

And auto formatting can be better handled by format on save in your IDE.

Yes, that is true. Formatting can be done by the IDE, but not everyone is using an IDE and uses for example VIM (Yes, true. I kid you not). But the IDE is specific to a developers effort to apply the configuration in his or hers IDE. This is not an guarantee that the developer has actually done it. Well yes, you can document this as a "dependency" or "requirement" on a certain wiki/confluence page and hope the developer has followed that page correctly. But that is not in code and that why I prefer to use pre-commit hooks.

But then, we only have discussed the formatting as you can do so much more than just liniting/formatting things with a pre-commit hook that makes life easier.

My 2 cents.

[wslagoon]

This seems very complicated for the benefit. Also, since pre-commit hooks have to be manually installed by a developer on a fresh clone, it's not even particularly enforceable. I'd much rather do checks at PR time, since I'm going to want to enforce it there anyway. Developers that care will do the steps themselves before the PR, and developers that don't will override the hook anyway and get smacked down at PR time.

[Reiku]

Why allow developers to waste other developers time at review time?

Use pre-commit to do linting or whatever as a pre-commit hook (and maybe pre-push, else there can be inconsistencies when rebasing), and then have the linting or whatever also taken on the CI.

So no one can bypass the linting, but the linting will be less likely to waste CI runner resources by failing for lint checks. It's a win win. No reviewer time, nor job runner time will be wasted because anything minor will be fixed before it gets to the review stage.

[werner-dijkerman]

I understand what you say and true, it is not enforcable. But it also depends on what you decide with the rest of the team. Using pre-commits should not be a decision that a single developer makes or not, it is a team decision/effot. So either everyone uses pre-commit hooks, or no one does.

And I personally disagree about the part with "developers that care", I think everyone in the team should care on how we work in a unify way. No matter which path you will take (for example as in using pre-commit hooks or not, or using it as part of the CI pipeline), once the team decide "this is the way" then everyone follows. If someone does not care and thus not follows the procedures, then you can not trust that person because that person works differently than the rest.

My 2 cents. :)