Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHPCS failing by default #2

Closed
ssnepenthe opened this issue Apr 13, 2017 · 7 comments

Comments

4 participants
@ssnepenthe
Copy link
Contributor

commented Apr 13, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

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 wp-cli/wp-cli#3728 and we still need to improve the autogenerated README wp-cli/scaffold-package-command#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 WordPress/WordPress-Coding-Standards#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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

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

@ssnepenthe ssnepenthe closed this Apr 14, 2017

@GaryJones

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

@GaryJones PHPCS used to only recognize ruleset.xml but that changed in a couple years ago: squizlabs/PHP_CodeSniffer@ae995b5

The name phpcs.ruleset.xml was adopted by wp-dev-lib which added the file back several years ago: xwp/wp-dev-lib@9d1d025

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

@danielbachhuber

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.