Configure pre-commit hook to only run required checks.

[kopepasah]

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.

[kasparsd]

@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:

wp-dev-lib/scripts/check-diff.sh

Lines 820 to 822 in e0205d7

	if [ ! -s "$TEMP_DIRECTORY/paths-scope-js" ]; then
	return
	fi

There is also a way to disable individual checks using the DEV_LIB_SKIP environment variable that is being checked during check_should_execute:

wp-dev-lib/scripts/check-diff.sh

Lines 1011 to 1021 in e0205d7

	function check_should_execute {
	if [ ! -z "$DEV_LIB_SKIP" ] && grep -sqi $1 <<< "$DEV_LIB_SKIP"; then
	return 1
	fi
	if [ ! -z "$DEV_LIB_ONLY" ] && ! grep -sqi $1 <<< "$DEV_LIB_ONLY"; then
	return 1
	fi
	return 0
	}

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.

[kopepasah]

@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.
• 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.

[kopepasah]

Ultimately, I think this needs further discussion and can be tackled in V2 of this project. 😃

[kasparsd]

@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:

wp-dev-lib/scripts/check-diff.sh

Lines 654 to 713 in e0205d7

	function run_phpunit_local {
	if [ ! -s "$TEMP_DIRECTORY/paths-scope-php" ]; then
	return
	fi
	if ! check_should_execute 'phpunit'; then
	echo "Skipping PHPUnit as requested via DEV_LIB_SKIP / DEV_LIB_ONLY"
	return
	fi
	# TODO: This should eventually run unit tests only in the state of the DIFF_HEAD
	(
	echo "## phpunit"
	if command -v phpunit >/dev/null 2>&1 && [ -n "$WP_TESTS_DIR" ]; then
	if [ -n "$PHPUNIT_CONFIG" ]; then
	phpunit $( if [ -n "$PHPUNIT_CONFIG" ]; then echo -c "$PHPUNIT_CONFIG"; fi )
	else
	for project in $( find_phpunit_dirs ); do
	(
	cd "$project"
	phpunit
	)
	done
	fi
	elif [ ! -z "$DOCKERFILE" ] && [ ! -z "$DOCKER_PHPUNIT_BIN" ]; then
	if [ -n "$PHPUNIT_CONFIG" ]; then
	"$DOCKER_PHPUNIT_BIN" -c "$PHPUNIT_CONFIG"
	else
	echo "Failed to run phpunit inside Docker"
	fi
	elif [ "$USER" != 'vagrant' ] && command -v vagrant >/dev/null 2>&1; then
	# Check if we're in Vagrant
	if [ ! -z "$VAGRANTFILE" ]; then
	cd $( dirname "$VAGRANTFILE" )
	VAGRANT_ROOT=$(pwd)
	if [ -e www/wp-content/themes/vip/plugins/vip-init.php ]; then
	ABSOLUTE_VAGRANT_PATH=/srv${PROJECT_DIR:${#VAGRANT_ROOT}}
	elif grep -q vvv Vagrantfile; then
	ABSOLUTE_VAGRANT_PATH=/srv${PROJECT_DIR:${#VAGRANT_ROOT}}
	fi
	cd - > /dev/null
	fi
	if [ ! -z "$ABSOLUTE_VAGRANT_PATH" ]; then
	VAGRANT_DEV_LIB_PATH=$ABSOLUTE_VAGRANT_PATH${DEV_LIB_PATH:${#PROJECT_DIR}}
	echo "Running phpunit in Vagrant"
	vagrant ssh -c "cd $ABSOLUTE_VAGRANT_PATH && export DIFF_BASE=$DIFF_BASE && export DIFF_HEAD=$DIFF_HEAD && export DEV_LIB_ONLY=phpunit && $VAGRANT_DEV_LIB_PATH/pre-commit"
	elif command -v vassh >/dev/null 2>&1; then
	echo "Running phpunit in vagrant via vassh..."
	vassh phpunit $( if [ -n "$PHPUNIT_CONFIG" ]; then echo -c "$PHPUNIT_CONFIG"; fi )
	else
	echo "Failed to run phpunit inside Vagrant"
	fi
	else
	echo "Skipping phpunit since not installed or WP_TESTS_DIR env missing"
	fi
	)
	}

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.