-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Simplify and document running phpcs locally
#986
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
Conversation
bin/run-phpcs.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: If phpcs.ruleset.xml is renamed to phpcs.xml.dist or phpcs.xml then you don't have to supply --standard at all. I suggest that this could eliminate the need for bin/run-phpcs.sh entirely. See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards#using-a-custom-ruleset
Additionally, the ruleset file can be configured to exclude the node_modules and vendor directories, eliminating the need for using find and xargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ruleset just needs to include these lines:
<exclude-pattern>*/node_modules/*</exclude-pattern>
<exclude-pattern>*/vendor/*</exclude-pattern>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'll try this out, and then move the setup documentation to a better place.
One more question: is it possible to enable the -s option via the config as well? I find it much easier to parse the output this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out we need to whitelist directories and files using the <file> tag instead.
With the suggested exclude entries in place, running phpcs -s . is extremely, extremely slow (2 minutes and counting before I killed it). strace reveals that phpcs still recurses into all directories, ignoring the exclude entries. This is a known issue, fixed in the 3.x releases, which we can't use yet: squizlabs/PHP_CodeSniffer#1113
is it possible to enable the
-soption via the config as well
From looking through the code, the answer to this is no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml I just found that you can indeed do this! See 7f0fff5.
Now all you need to do to run PHPCS is just do phpcs without any args at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
edit: Ha, I should have refreshed the page first, I would have seen the above ^^^ 😉
phpcs locallyphpcs locally
docs/coding-guidelines.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now remove -s from the instructions since it is defined as a default arg in phpcs.xml
.travis.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per below, the -s can now be removed since it is the default.
docs/coding-guidelines.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be easier to make phpcs 2.9.x a Composer dependency instead? If we add a composer.json which has wp-coding-standards/wpcs among its dev-requirements then all a contributor (and Travis CI alike) would need to do is composer install and then do ./vendor/bin/phpcs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this out, and am getting the following error:
PHP Fatal error: Uncaught exception 'PHP_CodeSniffer_Exception' with message 'Referenced sniff "WordPress-Core" does not exist'
Is it expected that we need to call vendor/bin/phpcs --config-set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nylen yes, that's right.
- Name the phpcs ruleset with a default filename - Whitelist PHP files to include in the checks
|
I'll merge this and explore using |
This PR makes it easier to run
phpcslocally.To test
phpcsstep, still passes.phpcsfollowing the instructions in the comments ofbin/run-phpcs.sh. Run the script and verify that it works (no output, exit code0).