WordPress/wporg-code-analysis

Contribute sniffs upstream

iandunn opened this issue · 5 comments

iandunn commented

Once the custom sniffs are mature, it might be a good idea to contribute them to the WordPress Coding Standards and WPThemeReview projects. They have broad application, and could be useful to many.

cc @jrfnl

jrfnl commented

@iandunn I've had a quick look through the repo. Here's some feedback to consider (or disregard, whatever takes your fancy):

  • For sniffs which are widely applicable, why not contribute them straight away to WPCS ? Especially as what I see so far in the MinimalPluginStandard directory seem (in part) rip-offs of the WPCS sniffs anyway.
    Improvements to WPCS are very welcome, though do expect them to be reviewed very critically.
    Atomic commits/PRs (incremental changes) will help get things accepted.
  • I see a dev requirement to PHPCSUtils, but as both sniffs appear to use it based on the use statements at the top of the files, this should be a non-dev require.
    You can then also remove the dealerdirect/phpcodesniffer-composer-installer dependency as it will be inherited from PHPCSUtils (with better version constraints).
  • I see numerous utility functions in the sniffs which could benefit from/could be simplified or even replaced by using utilities from PHPCSUtils.
  • I see a testVersion being set for PHPCompatibility in the ruleset and PHPCompatibilityWP as a require, but the PHPCompatibility[WP] ruleset is not included in the ruleset, so not run.
  • I see various duplicate and semi-invalid exclude-patterns in the ruleset.
  • I see the WordPress.NamingConventions.PrefixAllGlobals sniff being included, but without setting a prefix, this is useless.
  • I see a custom way of testing sniffs, while PHPCS has a build-in abstract test class available which simplifies the testing immensely.
    (Also: please don't ever use assertEquals() in tests unless you're testing objects).
  • etc...
iandunn commented

Thanks for the review and tips!

tellyworth commented
  • I see a custom way of testing sniffs, while PHPCS has a build-in abstract test class available which simplifies the testing immensely.

Where is the abstract test class documented? I was unable to work out how to use it, so I followed this example: https://payton.codes/2017/12/15/creating-sniffs-for-a-phpcs-standard/#writing-tests

(Also: please don't ever use assertEquals() in tests unless you're testing objects).

What should I use instead?

tellyworth commented

For sniffs which are widely applicable, why not contribute them straight away to WPCS

This is an experimental repo. It's fast moving and unstable, and as you've noted has plenty of problems that we have yet to solve. Once we've figured out what's possible and what's useful we definitely will contribute it to WPCS and elsewhere as applicable.

Thanks for the notes!

jrfnl commented

Where is the abstract test class documented? I was unable to work out how to use it.

See: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/tests/Standards/AbstractSniffUnitTest.php or just look at how WPCS tests sniffs.

If you can't get this working it may be due to the fact that the whole setup is broken as it doesn't comply with the PHPCS naming conventions and therefore will not work properly as a stand-alone standard.

You may want to have a read through the sniff writing tutorial to get a better understanding of the basics: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Coding-Standard-Tutorial

What should I use instead?

assertSame()