xwp
Configure pre-commit hook to only run required checks.
The Git pre-commit hook currently runs all checks no matter what files have changed and this is not efficient. For example, an update to a README.md file will run all linting, which is unnecessary for this change.
The pre-commit hook should be reworked to only run checks on file changes.
Optionally, we could explore removing this pre-commit hook in favor of a more robust tool like Husky.
@kopepasah dev-lib tries to detect which checks should run based on the files changed in the current changeset -- for example, before linting JS files it checks if any of the JS files were changed:
https://github.com/xwp/wp-dev-lib/blob/e0205d733c2cabccfd05ec281ae43872fc42c221/scripts/check-diff.sh#L820-L822
There is also a way to disable individual checks using the DEV_LIB_SKIP environment variable that is being checked during check_should_execute:
https://github.com/xwp/wp-dev-lib/blob/e0205d733c2cabccfd05ec281ae43872fc42c221/scripts/check-diff.sh#L1011-L1021
This allows you to disable and enable specific checks as needed. For example, see these examples in the readme for including and excluding specific checks.
@kasparsd thanks and that does make sense. I still think there is value in breaking out some of these checks, as asking engineers to set "skip files" in order to not perform some checks seems like a good case for automation.
The way I would like this pre-commit hook to behave is as follows:
- It is installed as a requirement when using this package.
- To me, it seems odd that we can have errors found in CI that could easily be caught in
pre-commit. This only creates more back-and-forth and confusion when trying to determine the cause of errors.
- To me, it seems odd that we can have errors found in CI that could easily be caught in
- It defaults to checking all STAGED changes.
- Further configuration can be implemented to check all files, but at a minimum, staged changes should ALWAYS be checked and not allow committing with errors.
- Secondly, it bases its checks on these staged changes, so changes to JavaScript files would run all JS checks, changes to PHP files would run all PHP checks and so on.
Furthermore, while the skip config is useful, it too can cause unforeseen problems. For example, let's say I setup a project and add the skip config to skip PHPUnit (maybe because I am not doing any PHP work on the project at the time). Six months go by and I am asked to work through some of the PHP side of the project, so I create some new namespaces, classes, tests, et cetera, et cetera. Then, I go to commit my changes and everything goes through without a problem, but alas... errors were thrown in CI when running the PHPUnit tests.
As someone familiar with the project, I may quickly realize that I skipped the unit tests at some point and forgot to "unskip" them later. I think we can tackle this a better way to always run specific tests based on the files being changed and committed. For example, my PHP changes would have automatically triggered PHPUnit based on the types of files being changed.
So, yes, I do understand that the DEV_LIB_SKIP config is a valuable piece of the puzzle, but I personally would like to see more of a plug-and-play type of functionality when it comes to the automated checks provided by wp-dev-lib.
Ultimately, I think this needs further discussion and can be tackled in V2 of this project. 😃
@kopepasah I think that what you describe is already the default behaviour -- it only runs the checks necessary and applicable to the types of files that were changed in the currently staged diff -- see all the check_should_execute calls in wp-dev-lib/scripts/check-diff.sh. Were you thinking of something extra?
The way I would like this pre-commit hook to behave is as follows: It is installed as a requirement when using this package.
This library does many things and it should be up to the user of the package to configure it because we have no way of making good assumptions about what their project needs are. For example -- are they using npm or Composer as the canonical task runner, are they using Travis or Circle CI, do they have other Git hooks installed that should be preserved, etc. This is similar to how pretty much all tools don't do anything without you defining what and when should be executed.
Then, I go to commit my changes and everything goes through without a problem, but alas... errors were thrown in CI when running the PHPUnit tests.
The default is to run phpunit checks if phpunit binary is found (either global or local under vendor/bin) and changes in PHP files have been detected:
https://github.com/xwp/wp-dev-lib/blob/e0205d733c2cabccfd05ec281ae43872fc42c221/scripts/check-diff.sh#L654-L713
So by default it will run those unit tests as soon as you add them. There is very little we can do if the users decide to exclude phpunit via DEV_LIB_SKIP since they made that decision and we should respect that.