Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lint:php:fix script #2111

Closed
westonruter opened this issue Apr 11, 2019 · 4 comments
Closed

Add lint:php:fix script #2111

westonruter opened this issue Apr 11, 2019 · 4 comments
Milestone

Comments

@westonruter
Copy link
Member

In the amp-stories-redux branch there is a lint:js:fix script and a lint:css:fix script. There should be one for PHP as well:

diff --git a/package.json b/package.json
index 53f72a63..e0342816 100644
--- a/package.json
+++ b/package.json
@@ -98,6 +98,7 @@
     "lint:js": "wp-scripts lint-js .",
     "lint:js:fix": "wp-scripts lint-js . -- --fix",
     "lint:php": "vendor/bin/phpcs",
+    "lint:php:fix": "vendor/bin/phpcbf",
     "start": "wp-scripts start",
     "test": "npm-run-all --parallel test:*",
     "test:php": "phpunit",

I tried this but it the phpcbf command unexpectedly returned a failure code even when all errors were fixed.

@swissspidy
Copy link
Collaborator

swissspidy commented Apr 12, 2019

Seems to work fine for me:

$ vendor/bin/phpcbf

No fixable errors were found

Time: 1 mins, 17.99 secs; Memory: 62MB


$ echo $?
0

@westonruter
Copy link
Member Author

When it actually fixed an error then it returned an error code. Running it a second time when there was nothing fixed rested in a success error code. Looks like we just need to do a check for the expected success error code and return it to NPM instead. See squizlabs/PHP_CodeSniffer#1359

@swissspidy
Copy link
Collaborator

I tried to do something like this:

"lint:php:fix": "vendor/bin/phpcbf || test $? -ne 1 || exit $?",

But then you can't run something like npm run lint:php:fix -- amp.php which I find quite useful, for example if you want to only target staged files.

If we'd want to run this on CI or as a pre-commit hook, then we should perhaps instead try keeping "lint:php:fix": "vendor/bin/phpcbf" and then add the exit code handling in a dedicated script or something.

@westonruter
Copy link
Member Author

@swissspidy I added a little wrapper Bash script to deal with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants