search

wp-cli / language-command

Installs, activates, and manages language packs.
MIT License
13 stars 21 forks source link

Implement CS checking based on the `WP_CLI_CS` ruleset #80

Closed jrfnl closed 5 years ago

jrfnl commented 5 years ago

This adds a PHPCS ruleset using the new WP_CLI_CS standard and adds some select whitelist comments.

Please see the individual commits for more detail.

Fixes #73

Related to wp-cli/wp-cli#5179

There are issues remaining for which I would like a second opinion on how to fix these:

FILE: src\Language_Namespace.php
------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 26 | ERROR | Classes declared by a theme/plugin should start with the theme/plugin
    |       | prefix. Found: "Language_Namespace".
    |       | (WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound)
------------------------------------------------------------------------------------------

:point_up: As this is not a command, I'm wondering if the class can be renamed.

FILE: src\WP_CLI\CommandWithTranslation.php
------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
------------------------------------------------------------------------------------------
  3 | ERROR | Namespaces declared by a theme/plugin should start with the theme/plugin
    |       | prefix. Found: "WP_CLI".
    |       | (WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedNamespaceFound)
 72 | ERROR | Object property "$Type" is not in valid snake_case format, try "$type"
    |       | (WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase)
 73 | ERROR | Object property "$Name" is not in valid snake_case format, try "$name"
    |       | (WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase)
 74 | ERROR | Object property "$Version" is not in valid snake_case format, try
    |       | "$version"
    |       | (WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase)
 77 | ERROR | Object property "$Language" is not in valid snake_case format, try
    |       | "$language"
    |       | (WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase)
------------------------------------------------------------------------------------------

:point_up: Regarding the object properties - these appear to be stdClass objects (based on visual code inspection only). If that's correct, I'm wondering if we can't just fix those rather than whitelist, though I'm open to whitelisting these.

FILE: src\WP_CLI\LanguagePackUpgrader.php
------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------
 3 | ERROR | Namespaces declared by a theme/plugin should start with the theme/plugin
   |       | prefix. Found: "WP_CLI".
   |       | (WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedNamespaceFound)
------------------------------------------------------------------------------------------

:point_up: Here and in the previous file - these files are not the commands, so I'm wondering if we can fix the namespace rather whitelist it.

schlessera commented 5 years ago

As this is not a command, I'm wondering if the class can be renamed.

Yes, it can be renamed or put into a proper namespace. However, that will make it inconsistent with the rest of the package, so I still think a whitelist is preferable here.

schlessera commented 5 years ago

Regarding the object properties - these appear to be stdClass objects (based on visual code inspection only). If that's correct, I'm wondering if we can't just fix those rather than whitelist, though I'm open to whitelisting these.

That's what we get back from a remote call to the wordpress.org API. I don't think we can change these.

schlessera commented 5 years ago

Here and in the previous file - these files are not the commands, so I'm wondering if we can fix the namespace rather whitelist it.

Same here again. Technically, we can indeed rename this, but it just make the package as a whole more inconsistent.

jrfnl commented 5 years ago

PR has been updated to reflect the above. I expect the build to pass now.

jrfnl commented 5 years ago

Hmm.. the build is failing hard, but I somehow don't think the failure is related to this PR. The failures are to do with the twenty-sixteen theme not being found. May just be a network connection hick-up which restarting the build would fix ?

schlessera commented 5 years ago

No, it's unrelated to this PR. My current guess is that the twenty-sixteen theme is not compatible with WP trunk right now. I've asked in #core-themes to see if they can tell me what is going on.

schlessera commented 5 years ago

Test failure unreleated to this PR and isolated to one single WP version. Merging anyway.