Introduce lint-staged #4441

kienstra · 2020-03-25T03:22:07Z

Summary

In package.json linting scripts, this only lints the diff from develop
~~Adds a pre-commit hook, using husky (maybe out of scope for this PR)~~
~~If we don't all want a pre-commit hook, maybe I should revert it. It's not optional in the way the wp-dev-lib pre-commit hook is. It's installed on npm install.~~
~~The pre-commit hook doesn't overwrite existing pre-commit hooks~~
~~Please do npm install to install the pre-commit hook~~

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests. (Manually tested, but unit testing doesn't really apply)
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

As Weston suggested, there's not much need to lint files that have no diff. This should run in Travis builds, as it does npm run lint.

kienstra · 2020-03-25T03:29:27Z

package.json

@@ -129,6 +130,11 @@
    "test:php": "phpunit",
    "test:php:help": "npm run test:php -- --help"
  },
+  "husky": {
+    "hooks": {
+      "pre-commit": "npm run lint"


Maybe this pre-commit hook is out of scope for this PR.

Yes, let's omit that for now.

Sure, I'll revert it

Removed in 740f3d1

kienstra · 2020-03-25T03:37:56Z

Hi Weston,
Hope you had a good day, no hurry with this.

kienstra · 2020-03-25T03:41:16Z

Sorry, not quite ready for review. If there's no diff to develop at all, it looks like npm run lint:php lints every PHP file.

As described in the issue, there's no need to lint a file with no diff to develop.

There probably isn't a need to copy the same command to 3 other places. So add a helper script.

It looks like most package.json scripts with a colon are for when there are multiple scripts that have the same prefix.

kienstra · 2020-03-25T05:18:51Z

package.json

    "lint:js:fix": "npm run lint:js -- --fix",
-    "lint:php": "vendor/bin/phpcs",
+    "lint:php": "npm run diff-develop | grep -E '\\.php$' | xargs vendor/bin/phpcs",


Maybe grep -E '\\.php$' is overkill as opposed to grep php

But grep -E '\\.js$' is probably better than simply grep js

kienstra · 2020-03-25T05:19:14Z

package.json

@@ -104,17 +105,18 @@
    "check-licenses": "wp-scripts check-licenses --production",
    "deploy": "grunt deploy",
    "dev": "wp-scripts start",
+    "diff-develop": "git diff develop --diff-filter=d --name-only",


--diff-filter=d prevents an error from linting a file that was deleted

package.json

kienstra · 2020-03-25T06:05:58Z

This is ready for review, sorry for that earlier.

westonruter · 2020-03-28T03:11:38Z

package.json

@@ -104,17 +104,18 @@
    "check-licenses": "wp-scripts check-licenses --production",
    "deploy": "grunt deploy",
    "dev": "wp-scripts start",
+    "diff-develop": "git diff develop --diff-filter=d --name-only",


Perhaps ls-changed-files would be more appropriate.

Alternatively, it may be better to just avoid having a separate command for this, and to instead just replace the instances of npm run diff-develop below with just the git diff command since it is short, and invoking git directly will reduce the need to fire up a subshell. If making this as fast as possible then this would be a consideration.

Sure, applied in 83e2155

package.json

westonruter · 2020-03-28T03:15:35Z

package.json

    "env:start": "./bin/local-env/start.sh",
    "env:stop": "./bin/local-env/stop.sh",
    "env:reset-site": "./bin/local-env/install-wordpress.sh --reset-site",
    "lint": "npm-run-all --parallel lint:*",
-    "lint:css": "wp-scripts lint-style",
+    "lint:css": "npm run diff-develop | grep -E 'css$' | xargs wp-scripts lint-style",
    "lint:css:fix": "npm run lint:css -- --fix",


Missing incorporation of the subset.

Sure, is 83e2155 what you had in mind by subset?

package.json

westonruter · 2020-03-28T03:21:24Z

package.json

    "lint:js:fix": "npm run lint:js -- --fix",
-    "lint:php": "vendor/bin/phpcs",
+    "lint:php": "npm run diff-develop | grep -E '\\.php$' | xargs vendor/bin/phpcs",


This isn't working properly when there are no changed PHP files. It is ending up checking all files. Perhaps it is because the phpcs.xml has <file>.</file>?

Hm, it looks like that might have been because npm run diff-develop also takes account of the changes in the develop branch, and this branch didn't have the latest from develop.

Before the commit below, here's the result of npm run diff-develop

bin/tag-built.sh composer.json composer.lock contributing/engineering.md includes/sanitizers/class-amp-base-sanitizer.php includes/validation/class-amp-validation-error-taxonomy.php lib/common/src/DevMode.php lib/common/src/Dom/Document.php lib/common/tests/DevModeTest.php lib/common/tests/Dom/DocumentTest.php package-lock.json package.json tests/php/test-amp-dev-mode-sanitizer.php tests/php/test-class-amp-base-sanitizer.php tests/php/test-tag-and-attribute-sanitizer.php tests/php/validation/test-class-amp-validation-error-taxonomy.php tests/php/validation/test-class-amp-validation-manager.php

Still, it's not good that it lints files that were changed in develop, as those aren't relevant here.

Co-Authored-By: Weston Ruter <[email protected]>

Instead of registering a new script, simply use that git command.

kienstra · 2020-04-01T04:19:26Z

Hi @westonruter,
Is your point from #4441 (comment) addressed?

It's not really ideal, but probably than linting all staged files.

Thanks, Weston!

google-cla · 2020-09-01T03:56:18Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pierlon · 2020-09-01T03:56:31Z

@googlebot I consent.

github-actions · 2020-09-01T03:59:11Z

Plugin builds for 2c85057 are ready 🛎️!

Download development build
Download production build

pierlon · 2020-09-01T04:01:31Z

This should be a small change that will contribute to the improvement of the DX experience. I've milestoned the PR and linked issue for v2.0.1 with the hopes that it can be reviewed and merged in time.

westonruter · 2020-09-03T02:46:15Z

package.json

    "lint:js:fix": "npm run lint:js -- --fix",
-    "lint:php": "./bin/get-changed-files.sh '\\.php$' | xargs -r ./vendor/bin/phpcs",
-    "lint:php:fix": "./bin/get-changed-files.sh '\\.php$' | xargs -r ./bin/phpcbf.sh",
+    "lint:php": "git diff HEAD^ --diff-filter=d --name-only | grep -E '\\.php$' | xargs -r ./vendor/bin/phpcs",


Why HEAD^ and not HEAD?

Also, invoking via xargs is going to mean that we won't be able to take advantage of <arg name="parallel" value="20"/> in the PHPCS config.

Why HEAD^ and not HEAD?

If I recall correctly it was because of renamed files not being listed when HEAD was used.

Nevertheless, it wouldn't be feasible changing these lint scripts here. It would break the workflow of linting the whole project (like in a CI environment, for example).

As discussed a few days ago, I think we can take advantage of a package called lint-staged, which gives us fine-grained control on what commands to run when certain files are staged. This would allow us to keep our original lint scripts intact, and with the combination of a pre-commit hook, we could have file changes being automatically linted whenever we're committing, for example.

See ab034bc for a basic setup of what I think is sufficient for the project.

schlessera · 2020-09-16T10:26:29Z

package.json

+    "**/*.css": [
+      "npm run lint:css:fix"
+    ],
+    "**/*.js": [
+      "npm run lint:js:fix"


Is this "fixing" linting issues here? If so, wouldn't that be incompatible with "staged changes only", as the changes it would make would then not be staged. So you fix something but won't commit that fix, unless you remember to use git add again.

Yes that makes sense. What's more is that when the --fix option is specified when linting JS or CSS files, all the eligible files are linted, instead of the staged ones. I'm not sure if it's a linter configuration error or bug with the linter itself.

I've removed the "fixing" of the lint issues in 76a9716.

westonruter · 2020-09-23T03:47:04Z

package.json

    "lint:php:fix": "./bin/phpcbf.sh",
+    "lint:plugin-bootstrap": "vendor/bin/phpcs --runtime-set testVersion 5.2- amp.php",


For some reason this command is failing for me, when invoked via lint-staged:

$ npm run lint:staged > amp-wp@ lint:staged /app/public/content/plugins/amp > lint-staged ✔ Preparing... ✔ Hiding unstaged changes to partially staged files... ⚠ Running tasks... ↓ No staged files match package.json [SKIPPED] ↓ No staged files match **/*.css [SKIPPED] ↓ No staged files match **/*.js [SKIPPED] ✔ Running tasks for **/!(amp).php ❯ Running tasks for amp.php ✖ npm run lint:plugin-bootstrap [FAILED] ↓ Skipped because of errors from tasks. [SKIPPED] ↓ Skipped because of errors from tasks. [SKIPPED] ✔ Reverting to original state because of errors... ✔ Cleaning up... ✖ npm run lint:plugin-bootstrap: PHP Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: Undefined index: in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Files/FileList.php on line 187 in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php:606 Stack trace: #0 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Files/FileList.php(187): PHP_CodeSniffer\Runner->handleErrors(8, 'Undefined index...', '/app/public/con...', 187, Array) #1 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(487): PHP_CodeSniffer\Files\FileList->current() #2 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() #3 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() #4 {main} thrown in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php on line 606 PHP Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: One or more child processes failed to run in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php:544 Stack trace: #0 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() #1 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() #2 {main} thrown in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php on line 544 npm ERR! code ELIFECYCLE npm ERR! errno 255 npm ERR! amp-wp@ lint:plugin-bootstrap: `vendor/bin/phpcs --runtime-set testVersion 5.2- amp.php "/app/public/content/plugins/amp/amp.php"` npm ERR! Exit status 255 npm ERR! npm ERR! Failed at the amp-wp@ lint:plugin-bootstrap script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above. npm ERR! A complete log of this run can be found in: npm ERR! /var/www/.npm/_logs/2020-09-23T03_46_30_988Z-debug.log > amp-wp@ lint:plugin-bootstrap /app/public/content/plugins/amp > vendor/bin/phpcs --runtime-set testVersion 5.2- amp.php "/app/public/content/plugins/amp/amp.php" Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: Undefined index: in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Files/FileList.php on line 187 in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php:606 Stack trace: #0 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Files/FileList.php(187): PHP_CodeSniffer\Runner->handleErrors(8, 'Undefined index...', '/app/public/con...', 187, Array) #1 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(487): PHP_CodeSniffer\Files\FileList->current() #2 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() #3 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() #4 {main} thrown in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php on line 606 Fatal error: Uncaught PHP_CodeSniffer\Exceptions\RuntimeException: One or more child processes failed to run in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php:544 Stack trace: #0 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php(114): PHP_CodeSniffer\Runner->run() #1 /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/bin/phpcs(18): PHP_CodeSniffer\Runner->runPHPCS() #2 {main} thrown in /app/public/content/plugins/amp/vendor/squizlabs/php_codesniffer/src/Runner.php on line 544 npm ERR! code ELIFECYCLE npm ERR! errno 1 npm ERR! amp-wp@ lint:staged: `lint-staged` npm ERR! Exit status 1 npm ERR! npm ERR! Failed at the amp-wp@ lint:staged script. npm ERR! This is probably not a problem with npm. There is likely additional logging output above. npm ERR! A complete log of this run can be found in: npm ERR! /var/www/.npm/_logs/2020-09-23T03_46_34_383Z-debug.log

Notice how it is invoking phpcs:

vendor/bin/phpcs --runtime-set testVersion 5.2- amp.php "/app/public/content/plugins/amp/amp.php"

Notice amp.php is being added twice.

Yea good catch. Addressed in 2c85057.

package.json

Co-authored-by: Weston Ruter <[email protected]>

The fix option has been removed.

In linting commands, only lint diff from develop

16d2a71

As Weston suggested, there's not much need to lint files that have no diff. This should run in Travis builds, as it does npm run lint.

googlebot added the cla: yes Signed the Google CLA label Mar 25, 2020

kienstra commented Mar 25, 2020

View reviewed changes

kienstra requested review from westonruter and removed request for westonruter March 25, 2020 03:38

kienstra added 3 commits March 24, 2020 22:12

If there is no diff to develop, don't lint

404b770

As described in the issue, there's no need to lint a file with no diff to develop.

Add a helper script to be more DRY

89ee859

There probably isn't a need to copy the same command to 3 other places. So add a helper script.

Change diff:develop to diff-develop

5461267

It looks like most package.json scripts with a colon are for when there are multiple scripts that have the same prefix.

kienstra commented Mar 25, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

kienstra requested a review from westonruter March 25, 2020 06:05

Remove pre-commit hook

740f3d1

westonruter requested changes Mar 28, 2020

View reviewed changes

kienstra and others added 3 commits March 29, 2020 12:57

Merge branch 'develop' into update/lint-changed-develop

7d35852

Commit Weston's suggestion for the regex of lint:css

4973402

Co-Authored-By: Weston Ruter <[email protected]>

Replace diff-develop script with simple git command

83e2155

Instead of registering a new script, simply use that git command.

amedina assigned kienstra Apr 1, 2020

schlessera added the WS:Core Work stream for Plugin core label Aug 18, 2020

pierlon added 2 commits August 31, 2020 18:11

Merge branch 'develop' into update/lint-changed-develop

4c40a75

Only run lint command when there are changes

6af326b

google-cla bot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Sep 1, 2020

google-cla bot removed the cla: no Has not signed the Google CLA label Sep 1, 2020

pierlon self-assigned this Sep 1, 2020

pierlon added this to the v2.0.1 milestone Sep 1, 2020

pierlon added 2 commits September 1, 2020 00:20

Remove external script

485bdfc

Detect file renames

c365250

westonruter requested changes Sep 3, 2020

View reviewed changes

westonruter modified the milestones: v2.0.1, v2.0.2 Sep 3, 2020

Use lint-staged package to perform linting on staged files

ab034bc

schlessera previously requested changes Sep 16, 2020

View reviewed changes

westonruter modified the milestones: v2.0.2, v2.0.3, v2.1 Sep 17, 2020

Only lint, not fix

76a9716

pierlon requested review from schlessera and westonruter September 19, 2020 03:41

westonruter reviewed Sep 23, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

Directly use phpcs command instead of lint:plugin-bootstrap npm script

2c85057

Co-authored-by: Weston Ruter <[email protected]>

westonruter approved these changes Sep 23, 2020

View reviewed changes

westonruter merged commit 047e28f into develop Sep 23, 2020

westonruter deleted the update/lint-changed-develop branch September 23, 2020 05:37

westonruter changed the title ~~In linting scripts, only lint diff from develop~~ Introduce lint-changed Sep 23, 2020

westonruter changed the title ~~Introduce lint-changed~~ Introduce lint-staged Sep 23, 2020

pierlon mentioned this pull request Sep 23, 2020

Run phpstan on staged PHP source files #5424

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce lint-staged #4441

Introduce lint-staged #4441

kienstra commented Mar 25, 2020 •

edited by pierlon

Loading

kienstra Mar 25, 2020 •

edited

Loading

westonruter Mar 25, 2020

kienstra Mar 25, 2020

kienstra Mar 25, 2020

kienstra commented Mar 25, 2020

kienstra commented Mar 25, 2020

kienstra Mar 25, 2020

kienstra Mar 25, 2020

kienstra commented Mar 25, 2020

westonruter Mar 28, 2020

westonruter Mar 28, 2020

kienstra Mar 29, 2020

westonruter Mar 28, 2020

kienstra Mar 29, 2020

westonruter Mar 28, 2020

kienstra Mar 29, 2020

kienstra Mar 29, 2020 •

edited

Loading

kienstra commented Apr 1, 2020

google-cla bot commented Sep 1, 2020

pierlon commented Sep 1, 2020

github-actions bot commented Sep 1, 2020 •

edited

Loading

pierlon commented Sep 1, 2020 •

edited

Loading

westonruter Sep 3, 2020

pierlon Sep 15, 2020

schlessera Sep 16, 2020

pierlon Sep 19, 2020

westonruter Sep 23, 2020

westonruter Sep 23, 2020

pierlon Sep 23, 2020

		"lint:php:fix": "./bin/phpcbf.sh",
		"lint:plugin-bootstrap": "vendor/bin/phpcs --runtime-set testVersion 5.2- amp.php",

Introduce lint-staged #4441

Introduce lint-staged #4441

Conversation

kienstra commented Mar 25, 2020 • edited by pierlon Loading

Summary

Checklist

kienstra Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Mar 25, 2020

kienstra commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

kienstra commented Apr 1, 2020

google-cla bot commented Sep 1, 2020

pierlon commented Sep 1, 2020

github-actions bot commented Sep 1, 2020 • edited Loading

pierlon commented Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Mar 25, 2020 •

edited by pierlon

Loading

kienstra Mar 25, 2020 •

edited

Loading

kienstra Mar 29, 2020 •

edited

Loading

github-actions bot commented Sep 1, 2020 •

edited

Loading

pierlon commented Sep 1, 2020 •

edited

Loading