-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Refactor suggestion / move utility functions from File to static classes #2189
Comments
Yeah, I think I like the idea. The File class has always been bloated but I've never put thought into refactoring it due to the large BC break it would cause.
My initial thought was v4 only, but your point about developers being able to migrate sniffs before v4 is a good one, so I think you are right. This feels like something that can go into v3 with the transitional methods and in File deprecated immediately and removed in v4.
I would normally throw them under a
They look ok to me. I don't have a strong opinion on this yet, but I imagine I will once I see some code being used.
This one's a big job and something I'd probably want to solely focus on for a single PHPCS release as it is also going to be a big information dump for sniff developers. So maybe this is the change in v3.5 or v3.6. |
@gsherwood Thanks for your response and feedback. I'd happily work on this, but as you say you'd prefer to have this in a dedicated release, I'll hold off for now until 3.4.0 has come out. That would also allow for PR #2128 to be merged before this refactor. |
I've been thinking about this some more and while traits would be a good solution for some of the external standards I work on, which have a base sniff class from which all sniffs extend, for PHPCS itself, the implementation will be simpler and cleaner as classes with static properties and static methods. This will prevent naming conflicts over imported traits when both an abstract sniff as well as a child sniff would use the same trait, or when one trait would use another. With that mind, putting these static classes in the For anyone wondering about the difference/choice for static classes: In some of the external standards, a base sniff class is used which makes those available via abstract class Sniff implements PHPCS_Sniff {
public $phpcsFile;
public $tokens;
public function process(File $phpcsFile, $stackPtr) {
$this->phpcsFile = $phpcsFile;
$this->tokens = $phpcsFile->getTokens();
return processToken($stackPtr);
}
abstract public function processToken($stackPtr);
} However, as this is not standard in PHPCS itself, traits would not have access to |
Just FYI: I've started work on this as, as soon as I started work on the AbstractDocblockSniff - #2222 -, I realized some things would be easier if some of the new utility functions would already be available. I hope to slowly but surely get a complete WIP branch ready by the time 3.4.0/3.4.1 is released, so I can start pulling the individual bits from it as soon as 3.5.0 opens for commits. Once I've cleaned up what I've done so far, I'll put a WIP branch online somewhere so anyone interested can have a look at the direction this is going in. |
Little status update for those interested: I'm working on this on and off, but making steady progress. I estimate that about 70% is done at this moment. Notes:
Current structure as things are shaping up:
Currently working on:
On the back-burner for later in combination with the
Feedback welcome. |
First thought... wow :) Second is just about naming. I don't think every class under I'd also change |
😀
Not existing convention or anything, just felt like a way to differentiate these classes from the other/existing classes in the Alternatively, another directory for these type of classes would also be an option. Just what would that directory be called in that case ?
That was a typo ;-) |
In my head, these were under File/Util, but I think I just read your comments incorrectly. Maybe Util/Sniffs ?? I hate naming things. |
To prevent further confusion, they are currently in the
I hear you. |
Ok, let's figure this name thing out as I will have to redo every single commit with the new names, so I can't really publish a WIP branch until we have a decision on this. Some suggestions of where to place these classes (in alphabetic order, not indicating any preference):
The previously suggested The previously suggested My preference would be a toplevel directory as hiding these away in a deeper level makes them less easily discoverable and they are kind of useful ;-) Watchers of this repo, please feel free to chime in with ideas or preferences as well. |
@gsherwood In practical terms, how would you like this to be pulled ? PR #2295 is a prerequisite for this PR so would need to be merged first and I've worked on the presumption that PR #2353 will be merged beforehand as well. The way I've set things up is with atomic commits with most of the time quite detailed commit messages telling "the story" (see screenshot). Some parts could be pulled simultaneously, but pulling things in one go or in blocks in the order I've set them up will probably be easiest as for implementation commits, the |
Of those choices, |
For the naming convention methods, I'm still trying to find unambiguous specifications. I've created a little comparison table of my current understanding of them. Input welcome! You can comment in the comparison table. |
@gsherwood Just checking in: have you had a chance to think about the naming and how to pull this ? |
@jrfnl Sorry, no. I'm swamped at work at the moment. |
@gsherwood Understood. When you do have some headspace to think it over, the naming/directory structure question is my first priority. All in all, things are shaping up nicely and, though I keep thinking of more things, I'd say I'm about 90% done. |
@gsherwood Quick question about the Now I have seen numerous issues related to this, so I'd like to account for this, which leaves me with a choice for which I need input:
As Squiz is a proprietary standard, I'd like know what you'd prefer. If 2️⃣, a What would you prefer ? |
I've never liked the fact that there are sniffs relying on the PHPCS core code to figure out short/long forms of types given preferences can differ between standards. I'd prefer to not have a method at all and let the sniffs decide themselves, but I realise that will result in a lot of repeated code. So I think the best thing to do is change the core to understand the standard formats from phpdoc and psr-5 and use those in the Squiz sniffs as well. It's a BC break for those sniffs, but I want the Squiz standard to change here anyway. I was waiting for PSR-5 to be completed, but I've been waiting a while now and I don't think it's going to happen. |
👍 Done ;-) Only question I still have would be whether the "long" form for |
Oh and I've also added a wrapper method Edit: Hmm... just thinking, that may not even be needed as |
Given true/false are valid return types, the long form probably remains as true/false, unless I've missed some complexity here. |
I think pulling in the other two commits at once makes sense. I'll target them for 3.5.0 as well. As for naming, I really dislike multi-word directories, so my vote is for |
@gsherwood Thanks for the input. That would hide these utility classes away in a deeper level, making them a little less easily discoverable, and - aside from the |
I don't think they are hidden in there. I suspect people would look in Utils for utility functions and see another directory dedicated to sniff utility classes. It also allows for future util categories to have a place.
True, but I don't think those other dirs are related to each other. This is a new collection of utility classes that will be used just as often as Util/Tokens. I'd prefer to keep them together in a Util folder and namespace. |
Okidokie, Give me a few days to adjust all the individual commits. After that I will set up a draft PR. |
Oh and just to be sure: we were going to drop the So, for just a sampling of the new classes:
|
Yes please |
Ok, done all the renames and recommits. WIP is up as a draft PR #2456 |
I don't think this is accurate. This snippet works just fine in PHP 5.5 and up: <?php
use NonExistentNamespace\NonExistentClass;
var_dump(class_exists(NonExistentClass::class)); |
@mwgamble Interesting, I'll do some testing with one of the external standards, but I seem to remember this was problematic when I was making some of those cross-version compatible with PHPCS 2 and 3. Providing the tests are successful, I'll remove that phrase from the Upgrade Guide. Thanks for your input! |
@mwgamble Tested & found working. I've updated the Upgrade Guide to reflect this and expanded the code sample as well. Thanks! |
Closing as this refactor is not going into PHPCS anymore. It has been published separately as PHPCSUtils. More details in this comment: #2456 (comment) |
Since PHPCS 3.x now has a minimum PHP requirement of PHP 5.4, I was wondering whether it might be an idea to refactor a number of groups of methods in the
File
class to traits.The reasoning behind this is two-fold:
File
class makes the re-useable code more modular and easier to maintain.Traits would allow for these methods to be added to PHPCS more easily without making the
File
class even larger than it already is.Timing
The transition could be made in PHPCS 3.x.
In PHPCS 3.x, the
File
class would in that caseuse
the new traits and for the transitioned methods just call the method in the trait under the hood.The methods in the
File
class would be deprecated and could be removed in PHPCS 4.x.This would allow external standards some time to implement the new way of doing things and make the BC break in PHPCS 4.x smaller as it can already be mitigated beforehand.
Proposal details
To be concrete, I'm currently thinking of the following Traits:
DeclarationName
, containing:getName()
, previously calledgetDeclarationName()
FunctionDeclaration
, containing:getParameters()
, previously calledgetMethodParameters()
getProperties()
, previously calledgetMethodProperties()
DeclarationName
for thegetName()
methodPropertyDeclaration
, containing:getMemberProperties()
ObjectDeclaration
containing:getClassProperties()
findExtendedClassName()
findImplementedInterfaceName()
findExtendedInterfaceNames()
- open PR File: add newFile::findExtendedInterfaceNames()
utility method #2128DeclarationName
for thegetName()
methodGeneric
containing:isReference()
ScopeConditions
, containing:hasCondition()
getCondition()
Additionally, I'd be happy to pull the following based on battle-tested code I've written for WPCS/PHPCompatibility:
FunctionCall
, containing:hasParameters()
getParameterCount()
getParameters()
getParameter()
(to get the details for the parameter at a specific position)These methods all work on both function calls as well as for splitting an array into the individual items and could be extended to cover
isset()
,unset()
andlist()
as well.FQClassNames
, containing:getNameFromNewToken()
getNameFromDoubleColonToken()
getExtendedName()
getFQName()
isNamespaced()
determineNamespace()
getDeclaredNamespaceName()
For the
Generic
trait, I could also offer an additionalisShortList()
method.I'm also still working on the logic for a
TStringAnalyser
which should reliably determine whether, for instance, aT_STRING
token represents a function call, the use of a global constant or for instance ause
statement alias.Open questions
What I'd currently like to know, is:
Traits
directory or, for instance, inUtil/Traits
or should the directory be called something completely different ?The text was updated successfully, but these errors were encountered: