search

wp-cli / scaffold-command

Generates code for post types, taxonomies, blocks, plugins, child themes, etc.
MIT License
166 stars 86 forks source link

PHPCS failing by default #2

Closed ssnepenthe closed 7 years ago

ssnepenthe commented 7 years ago

I am not sure if this belongs here - it looks like the wp scaffold plugin-tests command still exists in the main WP-CLI repo and that this package isn't officially available using wp package install. I assume that all future development will be happening here?

In any case - I have just started to take advantage of the wp scaffold plugin-tests command (super convenient, thanks for that) and I noticed that the PHPCS command run by travis fails by default. The problem is that PHPCS is run against the tests dir and the SampleTest class with a filename of test-sample.php violates the naming conventions defined in the WordPress coding standards.

screen shot 2017-04-13 at 11 09 19 am

This isn't a huge deal since the sample test will likely be deleted or renamed right away, but I assume users will follow the same conventions as WP-CLI and therefore continue to run into this issue.

Would it be beneficial to make some changes here?

A couple of possibilities:

1) Add an exclude pattern in phpcs.ruleset.xml to exclude the tests directory entirely. This would be my preference and is what I am doing personally since my tests tend to stray from the WordPress standards in at least a few ways. 2) Add per-rule exclude patterns for the tests directory. WordPress.Files.FileName.InvalidClassFileName is the rule needed to address this issue. Some other good candidates (in my opinion) would be Squiz.Commenting.FileComment.Missing, Squiz.Commenting.ClassComment.Missing and Squiz.Commenting.FunctionComment.Missing. 3) Update the sample test to better meet the WordPress standards. This would probably mean renaming the SampleTest class to Sample_Test, moving test-sample.php to class-sample-test.php and changing the PHPUnit testsuite definition to drop the test- prefix and update the suffix to -test.php. 4) ?

danielbachhuber commented 7 years ago

I am not sure if this belongs here - it looks like the wp scaffold plugin-tests command still exists in the main WP-CLI repo and that this package isn't officially available using wp package install. I assume that all future development will be happening here?

Yep! This is the proper place for it. The back story is in https://github.com/wp-cli/wp-cli/issues/3728 and we still need to improve the autogenerated README https://github.com/wp-cli/scaffold-package-command/issues/100

The problem is that PHPCS is run against the tests dir and the SampleTest class with a filename of test-sample.php violates the naming conventions defined in the WordPress coding standards.

See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/882

Would it be beneficial to make some changes here?

I'm open to being convinced otherwise, but my current plan was to wait it out. The behavior will change again in the next WPCS release.

ssnepenthe commented 7 years ago

Oh interesting - I never would have thought to look at wpcs for this. If it is being handled there, I won't push for one choice over another in this issue as I think this is more a matter of personal preference.

I do have some thoughts on the test scaffolding in general though - maybe this would be better discussed in a separate issue?

1) I am still not personally convinced that tests should be run against the full coding standards, maybe just a pared down set of rules if at all? In particular, a lot of the comment and naming rules seem unnecessary to me. I don't know if this should even be a consideration, but try checking the core test directory or even wp-cli core tests. 2) There don't seem to be any standards in the WordPress world when it comes to tests except those set by WP-CLI. It might be nice to bring these more in line with general PHPUnit standards (no prefix, suffix of Test.php or maybe -test.php). 3) On that note - prefix isn't part of the PHPUnit config schema (as mentioned by @GaryJones elsewhere). It doesn't even seem to be a documented feature in PHPUnit (I only checked the docs back to 4.8). Best I can tell is that it is part of the PHPUnit file iterator package, so maybe support is just included for convenience?

danielbachhuber commented 7 years ago

I do have some thoughts on the test scaffolding in general though - maybe this would be better discussed in a separate issue?

Nah, this is fine.

  1. I am still not personally convinced that tests should be run against the full coding standards, maybe just a pared down set of rules if at all? In particular, a lot of the comment and naming rules seem unnecessary to me. I don't know if this should even be a consideration, but try checking the core test directory or even wp-cli core tests.

At this point, I think this is a personal preference thing which you're welcome to disable. If we make any changes to the default phpcs.ruleset.xml, it should reflect "official" coding standards documented in the core handbook.

  1. There don't seem to be any standards in the WordPress world when it comes to tests except those set by WP-CLI. It might be nice to bring these more in line with general PHPUnit standards (no prefix, suffix of Test.php or maybe -test.php).

I'm not opposed to this, but I think those standards should be set for WordPress core, and then reflected in WP-CLI. I'm not terribly interested in debating which coding standards we should adopt at this point. @westonruter or @johnbillion may be good resources to facilitate this.

  1. On that note - prefix isn't part of the PHPUnit config schema (as mentioned by @GaryJones elsewhere). It doesn't even seem to be a documented feature in PHPUnit (I only checked the docs back to 4.8). Best I can tell is that it is part of the PHPUnit file iterator package, so maybe support is just included for convenience?

I think it's just a vestige from a long time ago.

ssnepenthe commented 7 years ago

Fair enough. I will close this then and it can be revisited if @westonruter or @johnbillion have any opinions on the matter.

GaryJones commented 7 years ago

Pinging @jrfnl as a FYI.

Related question @danielbachhuber - what's the reason for the naming as phpcs.ruleset.xml, instead of phpcs.xml.dist, as per the exact file in PHPCS, and in the documentation?

westonruter commented 7 years ago

@GaryJones PHPCS used to only recognize ruleset.xml but that changed in a couple years ago: https://github.com/squizlabs/PHP_CodeSniffer/commit/ae995b5bfd17cd1102fc5f8f93ae56f442b318e7

The name phpcs.ruleset.xml was adopted by wp-dev-lib which added the file back several years ago: https://github.com/xwp/wp-dev-lib/commit/9d1d0254c6bba84f84100bf9837d326ddb16032f

I believe WP-CLI, like wp-dev-lib, haven't been updated to account for PHPCS more recently supporting phpcs.xml.

danielbachhuber commented 7 years ago

Yep — that's the way it's been.