Closed coreymckrill closed 1 year ago
rrennick
commented
1 year ago @coreymckrill Is there something missing from the test instructions or is this a missing dependency. I got an error on husky:
> wc-smooth-generator@1.1.0 prepare /Users/ronrennick/Sites/wc/wp-content/plugins/wc-smooth-generator
> husky install
husky - Git hooks failed to install
husky - fs.copyFileSync is not a function
coreymckrill
commented
1 year ago @rrennick Hmm, looks like maybe a Node version incompatibility. Which version are you running and getting that error with?
rrennick
commented
1 year ago Hmm, looks like maybe a Node version incompatibility.
@coreymckrill If you had a bit of time it might be worth adding a .nvmrc file to facilitate switching versions between repos. The setup script works fine on node v16.
rrennick
commented
1 year ago It doesn't look like the linting is catching the errors after the pre-commit hook is installed:
# git diff > ../wcg.patch
~/Sites/wc/wp-content/plugins/wc-smooth-generator
# .git/hooks/pre-commit
Checking PHP Lint...
~/Sites/wc/wp-content/plugins/wc-smooth-generator
# cat ../wcg.patch
diff --git a/bin/lint-branch.sh b/bin/lint-branch.sh
index ed17883..72e271b 100644
--- a/bin/lint-branch.sh
+++ b/bin/lint-branch.sh
@@ -12,6 +12,6 @@
baseBranch=${1:-"trunk"}
changedFiles=$(git diff $(git merge-base HEAD $baseBranch) --relative --name-only -- '*.php')
-
+echo changedFiles
# Only complete this if changed files are detected.
[[ -z $changedFiles ]] || composer exec phpcs-changed -- -s --git --git-base $baseBranch $changedFiles
diff --git a/includes/Admin/Settings.php b/includes/Admin/Settings.php
index 83e6c09..29286ef 100644
--- a/includes/Admin/Settings.php
+++ b/includes/Admin/Settings.php
@@ -128,12 +128,12 @@ class Settings {
if ( filter_input( INPUT_POST, 'cancel_all_generations' ) ) {
$embed = 'NF9Y3GVuPfY';
} else {
- $videos = array(
+ $videos = [
'4TYv2PhG89A',
'6Whgn_iE5uc',
'h_D3VFfhvs4',
'QcjAXI4jANw',
- );
+ ];
$next_wait = filter_input( INPUT_COOKIE, 'smoothgenerator_next_wait' );
if ( ! isset( $videos[ $next_wait ] ) ) {
$next_wait = 0;
diff --git a/includes/Plugin.php b/includes/Plugin.php
index 2196632..4a09b73 100644
--- a/includes/Plugin.php
+++ b/includes/Plugin.php
@@ -17,7 +17,7 @@ class Plugin {
*
* @param string $file Main plugin __FILE__ reference.
*/
- public function __construct( $file ) {
+ public function __construct($file) {
if ( is_admin() ) {
Admin\Settings::init();
}@rrennick 🤔 It's working for me. I'm a little bit puzzled by your output above. Is this what you get when you try to commit?
# .git/hooks/pre-commit
Checking PHP Lint...This bit is weird, because the pre-commit hook script should not echo a message like this. Maybe you still have the old pre-commit hook installed as well?
rrennick
commented
1 year ago Maybe you still have the old pre-commit hook installed as well?
Here's the contents of the file. If this is the old hook, then this PR should be but isn't replacing it.
# cat .git/hooks/pre-commit
#!/bin/sh
PROJECT=`php -r "echo dirname(dirname(dirname(realpath('$0'))));"`
STAGED_FILES_CMD=`git diff --cached --name-only --diff-filter=ACMR HEAD | grep \\\\.php`
if [ ! -f ./phpcs.xml ] && [ ! -f ./phpcs.xml.dist ]; then
echo "./phpcs.xml or ./phpcs.xml.dist not found!"
exit 1;
fi
# Determine if a file list is passed.
if [ "$#" -eq 1 ]; then
oIFS=$IFS
IFS='
'
SFILES="$1"
IFS=$oIFS
fi
SFILES=${SFILES:-$STAGED_FILES_CMD}
# Run php lint.
echo "Checking PHP Lint..."
for FILE in $SFILES; do
php -l -d display_errors=0 $PROJECT/$FILE
if [ $? != 0 ]; then
echo "Fix the error before commit."
exit 1
fi
FILES="$FILES $PROJECT/$FILE"
done
# Run phpcs.
if [ "$FILES" != "" ]; then
echo "Running Code Sniffer."
./vendor/bin/phpcs --encoding=utf-8 -n -p $FILES
if [ $? != 0 ]; then
echo "Fix the error before commit!"
echo "Run"
echo " ./vendor/bin/phpcbf $FILES"
echo "for automatic fix or fix it manually."
exit 1
fi
fi
exit $?@rrennick I actually still have that old script in my .git/hooks directory as well. But what husky does is define a different directory for hooks via the .git/config file. Can you check your config file (local repo one, not your global one) and see if there's a hooksPath property under [core]? If it's not there, could you try running npm run setup again now that you're on a compatible version of Node?
coreymckrill
commented
1 year ago Ah, looks like you also need to have at least version 2.9 of Git for husky to work right.
rrennick
commented
1 year ago Can you check your config file (local repo one, not your global one) and see if there's a hooksPath property under [core]?
npm run setup worked properly and updated the git config. This is what's echoed to STDOUT when I commit:
chg=$(git diff HEAD --relative --name-only -- '*.php'); [[ -z $chg ]] || phpcs-changed -s --git $chgIt doesn't block the commit with changes that should be caught by linting.
coreymckrill
commented
1 year ago That is the correct output...
It doesn't block the commit with changes that should be caught by linting.
Are they the same changes to the two files shown in the output you posted here? I just tried making the same changes and got the following output (and the commit aborted):
> chg=$(git diff HEAD --relative --name-only -- '*.php'); [[ -z $chg ]] || phpcs-changed -s --git $chg
FILE: includes/Admin/Settings.php
-----------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 0 WARNINGS AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------
131 | ERROR | Short array syntax is not allowed (Generic.Arrays.DisallowShortArraySyntax.Found)
-----------------------------------------------------------------------------------------------
FILE: includes/Plugin.php
-----------------------------------------------------------------------------------------------
FOUND 3 ERRORS AND 0 WARNINGS AFFECTING 3 LINES
-----------------------------------------------------------------------------------------------
20 | ERROR | No space after opening parenthesis is prohibited (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterOpenParenthesis)
20 | ERROR | Expected 1 spaces after opening parenthesis; 0 found (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpacingAfterOpen)
20 | ERROR | No space before closing parenthesis is prohibited (WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceBeforeCloseParenthesis)
-----------------------------------------------------------------------------------------------
Script chg=$(git diff HEAD --relative --name-only -- '*.php'); [[ -z $chg ]] || phpcs-changed -s --git $chg handling the lint-staged event returned with error code 1If it's some other change, maybe we need to think about revising the ruleset...
rrennick
commented
1 year ago Are they the same changes to the two files shown in the output you posted https://github.com/woocommerce/wc-smooth-generator/pull/124#issuecomment-1601027897?
Yes.
Changes proposed in this Pull Request:
The Gruntfile has two commands, one for running the code sniffer (phpcs) and one for checking the text domain on gettext strings. The latter is not necessary because the code sniffer can also check text domains. By running phpcs via a Composer script instead, we can eliminate the Grunt dependency and all of its sub dependencies entirely, which reduces the problems associated with npm packages that frequently have vulnerabilities and need updating.
This also adds the phpcs-changed package and some related scripts (similar to the WooCommerce Core monorepo) so that the linter will only check file lines that have been modified instead of giving all code standard violations for the whole project.
Other changes:
setupscript to simplify setting things up after cloning the repoHow to test the changes in this Pull Request:
nvm useto ensure you're on a compatible version of Node.npm run setup. This should install all necessary npm and composer packages and set up the git hooks.