-
Notifications
You must be signed in to change notification settings - Fork 87
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
Update PHPCS default rule set #161
Changes from 5 commits
1e581ba
e4a3d1b
b64faae
d1047f2
c449067
4160c12
d0ac996
2f23c5c
c22db9a
64920e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
<?xml version="1.0"?> | ||
<ruleset name="WordPress Coding Standards based custom ruleset for your plugin"> | ||
<description>Generally-applicable sniffs for WordPress plugins.</description> | ||
|
||
<!-- What to scan --> | ||
<file>.</file> | ||
<exclude-pattern>vendor/</exclude-pattern> | ||
|
||
<!-- How to scan --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend adding a link here to the PHPCS wiki so people will know where to find more information about these options. You can either use the generic wiki link: https://github.com/squizlabs/PHP_CodeSniffer/wiki Or links to both these pages as good starting points for people:
|
||
<arg value="sp"/> <!-- Show sniff and progress --> | ||
<arg name="basepath" value="./"/><!-- Strip the file paths down to the relevant bit --> | ||
<arg name="colors"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a side-note: |
||
<arg name="extensions" value="php"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another potentially useful arg is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Comment to add along-side it: |
||
<arg name="parallel" value="8"/><!-- Enables parallel processing when available for faster results. --> | ||
|
||
<!-- Rules: Check PHP version compatibility --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation is off here. Tabs vs spaces. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the earlier comments, using I'd also recommend adding a link to the PHPCompatibilityWP repo as a comment, again to help people to more easily find out more info: https://github.com/PHPCompatibility/PHPCompatibilityWP Maybe also add a link for more info about what the You may also want to add a comment saying that people can change the |
||
<config name="testVersion" value="5.3-"/> | ||
<rule ref="PHPCompatibility"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a WordPress plugin/theme, then it can reference the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since that is outstanding, should this remain as is and implement a change for it with #63? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would most definitely recommend changing this already to use the |
||
|
||
<!-- Rules: WordPress Coding Standards --> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Along the same lines as some of my other comments, you may want to add a link here to the WPCS repo and/or to the WPCS wiki page about configuring sniffs: |
||
<config name="minimum_supported_wp_version" value="4.6"/> | ||
<rule ref="WordPress"> | ||
<exclude name="WordPress.VIP"/> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worth waiting until WPCS 1.0.0 drops, which already removes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will work either way, even if the WPCS VIP deprecation would not make it into 1.0.0, so I'd leave it for now. |
||
</rule> | ||
<rule ref="WordPress.NamingConventions.PrefixAllGlobals"> | ||
<properties> | ||
<!-- Value: replace the function, class, and variable prefixes used. Separate multiple prefixes with a comma. --> | ||
<property name="prefixes" type="array" value="my-plugin"/> | ||
</properties> | ||
</rule> | ||
<rule ref="WordPress.WP.I18n"> | ||
<properties> | ||
<!-- Value: replace the text domain used. --> | ||
<property name="text_domain" type="array" value="my-plugin"/> | ||
</properties> | ||
</rule> | ||
<rule ref="WordPress.WhiteSpace.ControlStructureSpacing"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is more a personal preference, than a recommended setting. I'm not sure if it belong in a scaffold. |
||
<properties> | ||
<property name="blank_line_check" value="true"/> | ||
</properties> | ||
</rule> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked at the scaffold in detail, but in case it also adds a PHPUnit test set-up, you may want to add an additional set of custom properties for that: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#custom-unit-test-classes |
||
</ruleset> |
This file was deleted.
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.
Be aware that
exclude-pattern
values are interpreted as regular expressions, so this snippet would exclude every directory which included the stringvendor/
, i.e./some-other-vendor/
would also be excluded.While this may be something which will rarely cause issues, you can stabilize this to just the Composer
vendor
directory by using something along the lines of^/vendor/*
or the slightly more generic./vendor/
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.
For more information about how the
exclude-pattern
s are interpreted, see: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignoring-files-and-foldersThere 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.
Darn... my bad. The more generic version should be
/vendor/
. The.
which I'd inadvertently placed in front of it will not be interpreted as root, but as a regex wildcard, so, while it shouldn't give any problems, it also isn't necessary.