Skip to content

Conversation

@jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Apr 21, 2019

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)
------------------------------------------------------------------------------------------

☝️ 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)
------------------------------------------------------------------------------------------

☝️ 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)
------------------------------------------------------------------------------------------

☝️ 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.

jrfnl added 2 commits April 21, 2019 14:00
... as that version contains the new WPCliCS PHPCS standard.
* `.distignore`: no need to distribute the phpcs/phpunit config files.
* `.gitignore`: Allow devs to overwrite config files for PHPUnit and PHPCS
    - The `.dist` files should be committed. However, for their personal use, devs can  overrule those files with versions without `.dist`.
        Those personal versions should never be committed.

As a side-note: for PHPCS, having a personal version while still using the original  `.dist` file is made very easy, as you can just import the `.dist` file as a starting  point, like so:
```xml
<?xml version="1.0"?>
<ruleset name="WP-CLI">
    <rule ref="./phpcs.xml.dist"/>
</ruleset>
```
@jrfnl jrfnl requested a review from a team as a code owner April 21, 2019 12:13
@schlessera
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 jrfnl force-pushed the feature/use-phpcs branch from ef7cfe1 to 7433d7c Compare April 21, 2019 12:44
@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 21, 2019

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

@jrfnl
Copy link
Contributor Author

jrfnl commented Apr 21, 2019

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
Copy link
Member

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
Copy link
Member

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

@schlessera schlessera merged commit 1e0b4a9 into wp-cli:master Apr 23, 2019
@schlessera schlessera added this to the 2.0.3 milestone Apr 23, 2019
@jrfnl jrfnl deleted the feature/use-phpcs branch April 23, 2019 12:33
schlessera pushed a commit that referenced this pull request Jan 5, 2022
* Composer: use wp-cli-tests ^2.1

... as that version contains the new WPCliCS PHPCS standard.

* .gitignore/.distignore: ignore phpcs/phpunit config files

* `.distignore`: no need to distribute the phpcs/phpunit config files.
* `.gitignore`: Allow devs to overwrite config files for PHPUnit and PHPCS
    - The `.dist` files should be committed. However, for their personal use, devs can  overrule those files with versions without `.dist`.
        Those personal versions should never be committed.

As a side-note: for PHPCS, having a personal version while still using the original  `.dist` file is made very easy, as you can just import the `.dist` file as a starting  point, like so:
```xml
<?xml version="1.0"?>
<ruleset name="WP-CLI">
    <rule ref="./phpcs.xml.dist"/>
</ruleset>
```

* PHPCS: add a ruleset for this project

The file is completely documented in-line.

* PHPCS: whitelist some code for select sniffs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adopt and enforce new WP_CLI_CS standard

2 participants