pre-commit: run only on changed files instead of all the files

[k3yss]

According to https://pre-commit.com , by default, the pre-commit hooks should only run on the edited files instead of all files. This PR addresses this and changes the default to the former.

Another thing we can provide is an option in devenv.nix to set the desired behavior.

[sandydoo]

Yeah, but you'd want to run the hooks against all of the files in a CI/test run. Hooks can fail to run ,or can be disabled and bypassed. This catches that.

[k3yss]

That makes sense, what about giving users an option to set the behavior of pre-commit hook while running devenv test? Do we already do this?

[kourtni]

Yeah, but you'd want to run the hooks against all of the files in a CI/test run. Hooks can fail to run ,or can be disabled and bypassed. This catches that.

I'm not certain why you believe that to be a best practice.

It certainly is not how companies with large code bases typically do things. Failing a CI pipeline because all existing or legacy code does not meet some new style or linting requirements that we want to begin introducing, doesn't really make sense. That would force many companies to spend months and years before they could successfully submit any code through their CI pipeline. Yes, we make certain that all code, existing and new, pass unit/integration/e2e testing (and even then it gets turned off for some instances there). But I've never heard of enforcing style enforcement on legacy code.

After 10+ years the pre-commit maintainers still default the tool to only run against edited files, with the option to run against all files by using the --all-files flag. And that has worked successfully for CI pipelines for all of that time. When integrating their tool into yours it would seem prudent to trust their years of experience and also to maintain familiarity for existing users.

[sandydoo]

In a CI environment, pre-commit would do absolutely nothing without the --all-files flag. There are no edited files to run against, unless you manually set that up.

I'm not certain why you believe that to be a best practice.

For example, from their own docs:

pre-commit run --all-files: run all the hooks against all the files. This is a useful invocation if you are using pre-commit in CI. (optional) Run against all the files ¶ ... consider running that in CI too.

And literally, a pre-commit dev: https://github.com/pre-commit-ci/issues/issues/90#issuecomment-926884551

Failing a CI pipeline because all existing or legacy code does not meet some new style or linting requirements that we want to begin introducing, doesn't really make sense.

This would be a huge red flag at a company. If you're going to introduce a new linter rule piece-meal, then you need to leverage file exclusions and configure the linter to only emit warnings until you get the codebase under control. Honestly, in modern times, you'd likely just run the formatter against the entire codebase and add auto-formatting in CI.

After 10+ years the pre-commit maintainers still default the tool to only run against edited files [...] When integrating their tool into yours it would seem prudent to trust their years of experience and also to maintain familiarity for existing users.

Trust? Decades of experience? Come on, you can't be serious. 😂 Is there a pre-commit cult I'm not privy too?

what about giving users an option to set the behavior of pre-commit hook while running devenv test?

IMO I could see this being somewhat useful if you're running devenv test locally. But I'm doubtful that it's worth the added complexity of making that distinction.

An option to toggle the pre-commit run in devenv test would probably more useful. Making it a permanent option kind of invites bad practices, but a devenv test --skip-git-hooks flag to disable hooks temporarily could be good?

[kourtni]

That part about "the best practice is to apply a tool all at once and get everything up to spec", I can only guess, but perhaps he meant that as in it is the optimal path if practical. Of course we would all like to do that, but it is often not practical. So from what I've seen, more often than not, it isn't done on non-trivial code bases because it can be costly in a lot of ways. Believe me, I would love to be able to just run linters/formatters on a large code base and everything just work perfectly, but there are lots of consequences to be had. This response to a question on the Software Engineering Stack Exchange points to some.

If you are like many companies, working with a large code base but don't have a dedicated team to oversee the rollout (and possible rollback) of a migration to your entire codebase, then you move incrementally. And you run linters/formatters on new code prior to it ever being merged.

Maybe I'm just out of the loop and there are now easily automated answers for all of these concerns? I don't know, I'm more of a software engineer than a DevOps guru.

I do know that the devenv pre-commit integration behaving differently from the official pre-commit makes it harder for me to use, which means that I'll have to continue to use the official pre-commit.

[sandydoo]

Again, you have all the tools at your disposal to make this work. Enabling a repo-wide hook that fails if run on certain files, then glossing over the fact that you have a misconfigured hook by just hoping you don't touch those files as you work is frankly a little ridiculous. I would re-evaluate the motivation for enabling a hook like this in the first place.

I do know that the devenv pre-commit integration behaving differently from the official pre-commit makes it harder for me to use, which means that I'll have to continue to use the official pre-commit.

We are not in any way "using a different default" to pre-commit. There is no default for this use-case. The only default there is for running hooks on staged files pre commit, which git-hooks.nix and devenv obviously do.

Anyways, we're just derailing this PR at this point.

This PR is about devenv test and whether it should run pre-commit against all files. Since the currently intended use is for CI-like purposes, the answer is yes. We might have a valid use-case for using devenv test interactively, in which case we might explore this further for performance reasons to reduce iteration time.