- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1
Update for WPCS 1.0 release #8
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
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.xmlfile.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.jsonfile here so that we can take advantage of thephpcodesniffer-standardproject type, which facilitates auto-installation of rulesets when thedealerdirect/phpcodesniffer-composer-installerpackage 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.