-
Notifications
You must be signed in to change notification settings - Fork 1
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 for WPCS 1.0 release #8
base: master
Are you sure you want to change the base?
Conversation
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.
@JPry Thanks for the review request. It was a learning experience.
Within limits of my understanding, I have tried to review this.
Prospress-WC/ruleset.xml
Outdated
<ruleset name="Prospress-WC"> | ||
<description>Prospress WooCommerce Coding Standards</description> | ||
|
||
<!-- Include the main Prospress stnadard --> |
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.
🆎 stnadard -> standard.
Prospress-WC/ruleset.xml
Outdated
<exclude-pattern>*/templates/emails/plain/*</exclude-pattern> | ||
<properties> | ||
<!-- e.g. body_class, the_content, the_excerpt --> | ||
<property name="customAutoEscapedFunctions" type="array" value="woocommerce_wp_select,wcs_help_tip"/> |
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.
💡 This release note says that the above form of specifying array values has been deprecated and element
tags should be used. Not sure if it is only in the case of key-value pairs.
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.
Thanks for finding that, I didn't know that was the case 🙂
Prospress-WC/ruleset.xml
Outdated
<!-- e.g. body_class, the_content, the_excerpt --> | ||
<property name="customAutoEscapedFunctions" type="array" value="woocommerce_wp_select,wcs_help_tip"/> | ||
<!-- e.g. esc_attr, esc_html, esc_url--> | ||
<property name="customEscapingFunctions" type="array" value="wcs_json_encode,htmlspecialchars,wp_kses_allow_underscores"/> |
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.
💡 Setting array values using element
tag - same as above
<rule ref="PEAR.Functions.FunctionCallSignature.CloseBracketLine"> | ||
<severity>0</severity> | ||
</rule> | ||
<!-- Selectively import other WordPress rules --> |
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.
❔ Any particular reason why Generic.Files.LowercasedFilename
is not imported?
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.
Not all of our libraries (mainly Action Scheduler) use lowercase file names. Also, in my opinion, that's not one that we necessarily need to follow.
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.
Oh, you're also asking why I didn't keep it when it used to be part of the ruleset? The reason for that is that it appeared to be superseded by the WordPress.Files.FileName
rule.
<property name="customEscapingFunctions" type="array" value="0=>wcs_json_encode,1=>htmlspecialchars,2=>wp_kses_allow_underscores"/> | ||
<!-- e.g. _deprecated_argument, printf, _e--> | ||
<property name="customPrintingFunctions" type="array" value=""/> | ||
<property name="strict_class_file_names" value="false"/> |
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.
❔ As far as I have observed in WCS code, I see class file names starting with "class-". Any reason why this has not been enforced strictly?
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.
Not all of our libraries use the class-
prefix in file naming.
Thanks for the review @menakas! I've made some adjustments based on your feedback. |
This updates our ruleset to take into account the recent release of WPCS 1.0. With this update, I've tried to keep our existing rules in place by going through what we had to start with, and what is now found in the
ruleset.xml
file.One major difference is that the extended rules related to working with WC have been moved to a separate ruleset that builds on the main ruleset. Plugins such as Subscriptions should be able to use this, and we can use the main ruleset for our (very few) plugins that don't require WC.
I've added a
composer.json
file here so that we can take advantage of thephpcodesniffer-standard
project type, which facilitates auto-installation of rulesets when thedealerdirect/phpcodesniffer-composer-installer
package is used.Closes #5.
Closes #6.
Also, in case anyone is interested, here's the link to the documentation on how WPCS recommends customizing the rules: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties.