Semi-broken pre-commit hook configuration due to custom Git diff
Opened this issue · 1 comments
Thanks a lot for fixing #70 regarding the file globbing support. As requested, I'm hereby opening up a new issue about the other somewhat related issue I noticed, specifically that the existing pre-commit hook in this repository is semi-broken.
The reason behind the incorrect behaviour is that
Line 5 in 32b8805
--git-diff, which triggers the git-diff logic as implemented by relint. This might work in some cases, but not in all of them.
As an example, if you run pre-commit run --all-files, relint will say that everything passed, but did not actually end up checking anything. While pre-commit will always pass a list of files to check to each configured linter, the pre-commit hook in this repository unfortunately adds its own logic on top of that with --git-diff. While relint will in fact scan all files as expected by pre-commit, the custom logic throws away all results, as there are no changes in the repository, resulting in relint always attesting that everything passes.
This can be easily fixed by simply removing the --git-diff argument from the .pre-commit-hooks.yaml. IMHO this is also the more correct approach, as pre-commit already decides which files to check, so I'd expect all linters to just lint the files they've actually been passed. Any additional custom logic on top is rather dangerous, as now the various linters no longer check the same set of files.
Could you please consider making this change and simply use the files passed by pre-commit as-is? I think the --git-diff helper is a legitimate and useful utility for standalone users, so I would definitely keep the feature around, but I think using it for pre-commit is not a good approach for the reasons outlined above. Thanks a lot in advance for looking into this!
Hi @ppmathis,
Thanks again for opening a separate issue about this. I was contemplating the different scenarios. I don't use pre-commit run --all-files myself too often, but it is a very valid use case.
However, I think it's important to understand the differences. --git-diff will run changes not based on filename, but individually changed lines in your diff. I have projects with huge translation files, where we enforce or suggest incremental improvements.
That being said, arguments can be defined by projects, which implement a pre-commit hook. We shouldn't provide any defaults, that cause unexpected behavior and side effects.
Feel free to open a PR to change the default. Maybe even add a bit of documentation about the --git-diff argument.
Cheers!
Joe