Fix lint-staged running phpcs checks for standalone plugins #1089
Conversation
westonruter
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| "wp-env": "wp-env", | ||
| "prepare": "husky install" | ||
| }, | ||
| "lint-staged": { |
There was a problem hiding this comment.
@westonruter Since there are multiple entries for lint-staged, should we switch to .lintstagedrc.json?
Also, if we add .lintstagedrc.json in each plugin, then it will auto-resolve the staged files in that particular directory. For the experiment I put plugins/webp-uploads/.lintstagedrc.json and when I made changes to the files in plugins/webp-uploads it automatically ran on staged files within plugins/webp-uploads.
// path: plugins/webp-uploads/.lintstagedrc.json
{
"*.php": [
"composer run-script lint --working-dir=../../",
"composer run-script phpstan --working-dir=../../"
],
"*.js": [
"npm run lint-js"
]
}output:
➜ performance git:(trunk) ✗ git commit -m "Temp commit"
✔ Preparing lint-staged...
✔ Preparing lint-staged...
❯ Running tasks for staged files...
❯ plugins/webp-uploads/.lintstagedrc.json — 1 file
❯ *.php — 1 file
✔ composer run-script lint --working-dir=../../
✖ composer run-script phpstan --working-dir=../../
↓ *.js — no files [SKIPPED]
◼ Applying modifications from tasks...
◼ Cleaning up temporary files...
``There was a problem hiding this comment.
Hummm, this doesn't seem to work. I tried putting that .lintstagedrc.json at plugins/webp-uploads. I then added an intentional PHPCS failure, like putting the wrong text domain in a translation function. It didn't block the commit. I think this is because plugins is still excluded in the root phpcs.xml.dist. No PHPCS errors were reported. The PHPStan checks are running successfully, however.
There was a problem hiding this comment.
To get it to work, I have to explicitly run lint:webp-uploads and not just lint:
{
"*.php": [
"composer run-script lint:webp-uploads --working-dir=../../",
"composer run-script phpstan --working-dir=../../"
],
"*.js": [
"npm run lint-js"
]
}However, I'm not sure this is better than having a single root config because only the lint command is plugin-specific currently. Having plugin-specific .lintstagedrc.json files would introduce more duplication.
There was a problem hiding this comment.
However, I'm not sure this is better than having a single root config because only the lint command is plugin-specific currently. Having plugin-specific .lintstagedrc.json files would introduce more duplication.
Humm and it's a long-known trade-off in mono repos. It will bring duplication but lead to simpler management on plugin level but we can have a root level .lintstagedrc.json for now.
There was a problem hiding this comment.
Did some experiments on #1090 with dynamic lint-staged config.
@westonruter Can you please test it?
| "./{plugins/auto-sizes,tests/plugins/auto-sizes}/**/*.php": [ | ||
| "composer run-script lint:auto-sizes" | ||
| ], | ||
| "./{plugins/dominant-color-images,tests/plugins/dominant-color-images}/**/*.php": [ | ||
| "composer run-script lint:dominant-color-images" | ||
| ], | ||
| "./{plugins/embed-optimizer,tests/plugins/embed-optimizer}/**/*.php": [ | ||
| "composer run-script lint:embed-optimizer" | ||
| ], | ||
| "./{plugins/optimization-detective,tests/plugins/optimization-detective}/**/*.php": [ | ||
| "composer run-script lint:optimization-detective" | ||
| ], | ||
| "./{plugins/speculation-rules,tests/plugins/speculation-rules}/**/*.php": [ | ||
| "composer run-script lint:speculation-rules" | ||
| ], | ||
| "./{plugins/webp-uploads,tests/plugins/webp-uploads}/**/*.php": [ |
There was a problem hiding this comment.
These can be simplified once #1065 is merged.
| "./{plugins/auto-sizes,tests/plugins/auto-sizes}/**/*.php": [ | |
| "composer run-script lint:auto-sizes" | |
| ], | |
| "./{plugins/dominant-color-images,tests/plugins/dominant-color-images}/**/*.php": [ | |
| "composer run-script lint:dominant-color-images" | |
| ], | |
| "./{plugins/embed-optimizer,tests/plugins/embed-optimizer}/**/*.php": [ | |
| "composer run-script lint:embed-optimizer" | |
| ], | |
| "./{plugins/optimization-detective,tests/plugins/optimization-detective}/**/*.php": [ | |
| "composer run-script lint:optimization-detective" | |
| ], | |
| "./{plugins/speculation-rules,tests/plugins/speculation-rules}/**/*.php": [ | |
| "composer run-script lint:speculation-rules" | |
| ], | |
| "./{plugins/webp-uploads,tests/plugins/webp-uploads}/**/*.php": [ | |
| "./plugins/auto-sizes/**/*.php": [ | |
| "composer run-script lint:auto-sizes" | |
| ], | |
| "./plugins/dominant-color-images/**/*.php": [ | |
| "composer run-script lint:dominant-color-images" | |
| ], | |
| "./plugins/embed-optimizer/**/*.php": [ | |
| "composer run-script lint:embed-optimizer" | |
| ], | |
| "./plugins/optimization-detective/**/*.php": [ | |
| "composer run-script lint:optimization-detective" | |
| ], | |
| "./plugins/speculation-rules/**/*.php": [ | |
| "composer run-script lint:speculation-rules" | |
| ], | |
| "./plugins/webp-uploads/**/*.php": [ |
|
Closing in favor of #1090 |
I found that lint-staged was completely skipping phpcs checks for PHP files in standalone plugins. This is because it was attempting to run
composer run lint-stagedon files in thepluginsdirectory, which is excluded by thephpcs.xml.distconfig. This fixes the problem by adding plugin-specific patterns for lint-staged.