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

jrfnl · 2019-04-17T11:59:38Z

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

Fixes #158

Related wp-cli/wp-cli#5179

composer.json

jrfnl · 2019-04-17T15:28:21Z

Remaining issues list:
20190417-phpcs-result-i18n.txt

Once these issues have been resolved, either by fixing them, whitelisting individual issues or pinging me to adjust the ruleset, this PR will need to be rebased.

After that it can be marked Ready for review and the build should go green.

... 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> ```

The file is completely documented in-line.

schlessera · 2019-04-24T15:15:44Z

@jrfnl I pushed changes into this PR, it should now pass PHPCS.

jrfnl · 2019-04-24T15:38:29Z

@schlessera Thanks. I have a couple of remarks, but it seems that @swissspidy already merged this....

jrfnl · 2019-04-24T15:40:03Z

phpcs.xml.dist

+		<exclude-pattern>*/src/IterableCodeExtractor\.php$</exclude-pattern>
+	</rule>
+
+	<!-- Whitelist variable names for classes that extend a PSR-2 package. -->


It would be better to use the specific configuration for this sniff to allow for these.
See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#mixed-case-property-name-exceptions

@jrfnl There was actually three different ones that were triggered, so I didn't bother listing them individually anymore.

The problem with that is that now newly introduced variable name violations in these files will no longer be reported, which is why I suggested using the property.

Oh, I'm sorry, I should have actually read the link you posted. So instead of whitelisting against the rule, I should whitelist against the individual variable names.

Yes, I agree that that makes more sense, I'll push a subsequent PR.

Hmm, that only seems to work for properties, not variables. Am I missing a specific notation?
Here's what seems to work for the properties:

This leaves me with the following errors:

Having a look now at the files in question. This is what I see:

src/JSFunctionsScanner.php: I see no reason why the $extractComments property hasn't been renamed. It is a private property, so independent of the parent class. Why should this not comply ?

src/MapCodeExtractor.php: $mapObject is a function local variable, so it is perfectly safe to rename it. Why should this not comply ?

src/MapCodeExtractor.php: sourcesContent is a property and comes from an external source. This can be whitelisted via the above.

src/PhpFunctionsScanner.php: $extractedComment is a function local variable, so it is perfectly safe to rename it. Why should this not comply ?

So basically this would be the ruleset config. Everything else should just be renamed:

<rule ref="WordPress.NamingConventions.ValidVariableName"> <properties> <property name="customPropertiesWhitelist" type="array"> <element value="sourcesContent"/> </property> </properties> </rule>

You're right, I did these changes in #161

swissspidy · 2019-04-24T15:40:08Z

Whoops 🙃 Sorry about that.

jrfnl · 2019-04-24T15:42:15Z

src/IterableCodeExtractor.php


 		foreach ( $matchers as $path_or_file ) {
-			$pattern = preg_quote( str_replace( '*', '__wildcard__', $path_or_file ) );
+			$pattern = preg_quote( str_replace( '*', '__wildcard__', $path_or_file ), '/' );


This disregards the bug reported in #117 - MBString regexes don't use delimiters and using preg_quote() on a regex to be passed to one of the MBString regex functions is risky and potentially buggy.

@jrfnl Noted. We have the bug report open, so the issue won't be forgotten. I assume it is a bigger refactor to fix this, though. We're not worse off for now with the above code, as the bug was detected and recorded for a later fix.

schlessera · 2019-04-24T15:42:23Z

@schlessera Thanks. I have a couple of remarks, but it seems that @swissspidy already merged this....

Just post the remarks here, and I'll follow up in a separate PR as needed.

jrfnl · 2019-04-24T15:42:59Z

I just have ;-)

schlessera · 2019-04-24T15:45:10Z

I replied to them. Do you agree with my assessments, or is there something I did not consider?

jrfnl · 2019-04-24T15:47:47Z

@schlessera I agree with the one, not so much the other ;-)

schlessera · 2019-04-24T15:50:14Z

The mb_ereg/preg_quote mismatch, I assume.

I'm happy to look into actually fixing the bug for this release if I end up having time once the blocking bugs are fixed. This one hasn't bitten anyone yet and doesn't block the release, so it is low priority for me right now.

Implement CS checking based on the `WP_CLI_CS` ruleset

schlessera requested changes Apr 17, 2019

View reviewed changes

composer.json Outdated Show resolved Hide resolved

jrfnl force-pushed the feature/use-phpcs branch 2 times, most recently from 5b6f22a to 21e20b6 Compare April 17, 2019 15:24

jrfnl force-pushed the feature/use-phpcs branch from 21e20b6 to f2e4ebb Compare April 17, 2019 16:34

jrfnl mentioned this pull request Apr 17, 2019

Adopt and enforce new WP_CLI_CS standard #158

Closed

3 tasks

jrfnl added 3 commits April 18, 2019 21:41

Composer: use wp-cli-tests ^2.1

ca6f9b6

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

PHPCS: add a ruleset for this project

e09e5e9

The file is completely documented in-line.

jrfnl force-pushed the feature/use-phpcs branch from f2e4ebb to e09e5e9 Compare April 19, 2019 13:52

schlessera changed the title ~~Implement CS checking based on the WPCliCS ruleset~~ Implement CS checking based on the WP_CLI_CS ruleset Apr 20, 2019

schlessera added 4 commits April 24, 2019 17:12

Provide second argument to preg_quote()

3f4e386

Whitelist use of leading underscore in method name

0b8af6f

Whitelist code comment

362712f

Add whitelist rules to deal with the PSR-2-extending classes

357f304

schlessera marked this pull request as ready for review April 24, 2019 15:14

schlessera requested a review from a team as a code owner April 24, 2019 15:14

schlessera added the command:i18n label Apr 24, 2019

schlessera added this to the 2.1.1 milestone Apr 24, 2019

schlessera approved these changes Apr 24, 2019

View reviewed changes

swissspidy merged commit e4edd2f into wp-cli:master Apr 24, 2019

jrfnl commented Apr 24, 2019

View reviewed changes

jrfnl deleted the feature/use-phpcs branch April 24, 2019 15:43

schlessera mentioned this pull request Apr 24, 2019

Fine-tune property and variable name whitelisting #161

Merged

schlessera pushed a commit that referenced this pull request Jan 6, 2022

Implement CS checking based on the WP_CLI_CS ruleset (#156)

f2e31cf

Implement CS checking based on the `WP_CLI_CS` ruleset

Implement CS checking based on the WP_CLI_CS ruleset #156

Implement CS checking based on the WP_CLI_CS ruleset #156

Uh oh!

Conversation

jrfnl commented Apr 17, 2019 • edited by schlessera Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Composer: use wp-cli-tests ^2.1

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

PHPCS: add a ruleset for this project

Uh oh!

Uh oh!

jrfnl commented Apr 17, 2019

Uh oh!

schlessera commented Apr 24, 2019

Uh oh!

jrfnl commented Apr 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrfnl Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swissspidy commented Apr 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera commented Apr 24, 2019

Uh oh!

jrfnl commented Apr 24, 2019

Uh oh!

schlessera commented Apr 24, 2019

Uh oh!

jrfnl commented Apr 24, 2019

Uh oh!

schlessera commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

jrfnl commented Apr 17, 2019 •

edited by schlessera

Loading

jrfnl Apr 24, 2019 •

edited

Loading