The future of the `WordPress_Sniff` class #765

Open
jrfnl opened this Issue Jan 1, 2017 · 2 comments

Projects

None yet

2 participants

@jrfnl
Contributor
jrfnl commented Jan 1, 2017

As since #696, #703 and #633 have all been addressed & merged, nearly all sniffs which extended an upstream sniff have been reverted to stand-alone sniffs, I'm wondering if it's time to review the WordPress_Sniff class.

That class currently contains mostly static properties so other sniffs - which couldn't extend it - could use these.
It also contains a number of non-static methods (and a few such properties).

I intend to start adding a number of additional utility methods to this class in the near future to be used by a plentitude of sniffs.
These utility methods have been extensively tested over the last few months within the PHPCompatibility standard (I wrote them, so there is no licencing issue with bringing them over).
They will address things like a standardized way to determine if a T_STRING is a function call, getting the parameters from the function call, safely stripping the quotes off a T_CONSTANT_ENCAPSED_STRING, checking if something is within a namespace, getting the fully qualified function/method name etc

I imagine there might be advantages in:

  • turning a number of sniffs (quite a few actually) into extended classes based on the WordPress_Sniff class
  • turning the static properties into normal class properties
  • changing the init() method to process() and once it has stored the basic information, letting it hand off to a processToken() method which would be implemented in the child classes instead of the current process() method. This would save having to pass around the $phpcsFile and retrieving the $tokens in nearly every function as they would be available as class properties instead.

Opinions ?

@jrfnl jrfnl added the Type: Question label Jan 1, 2017
@grappler
Contributor
grappler commented Jan 1, 2017

Looking forward to this.

changing the init() method to process() ...

Would this not break backward compatibility? Which version are we aiming for here?

@jrfnl
Contributor
jrfnl commented Jan 1, 2017

Would this not break backward compatibility?

No, it would not.
PHPCS looks for the process() method (and has been doing this since PHPCS 1.x). At this moment it finds the process() method in each individual sniff. With what I'm suggesting, PHPCS would find still find that method, but now in the parent class (for those sniffs where we would implement this).
The child class can still provide it's own process() overloading the parent method if needed.
One advantage would be that we wouldn't have to call $this->init() (method in the WordPress_Sniff class) from within the individual sniffs, but that that would be done by default, making the $phpcsFile and $tokens properties available througout instead of having to pass them along/calling the $phpcsFile->getTokens() method from each individual function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment