You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is basically the refactoring I had in mind in #1362
The current state is already a bit weird IMO, in general trait props and methods were ignored when gathering class nodes. Then recently I added trait props to a dedicated array, just to use them also via uninitialized props checks. I did not add them to the normal props because that would lead to annoying new errors via UnusedPrivatePropertyRule.
Then just yesterday I added trait property promotion usage to the normal property usage, but ignoring other potential trait prop usages elsewhere. And trait methods are still completely ignored.
So what does this do different now?
Trait props and methods are gathered together with their class counterparts. And rules just use that. Additionally the information if the prop or method came from a trait is also remembered and only that is used to skip trait props / methods in those 2 UnusedPrivate* rules, that were annoying.
I think in the long-run this makes more sense and avoids more weird trait problems. But maybe the details could be modified a bit. WDYT?
Aah I guess I could keep BC here still if really needed ;)
I added that method in #1340 which was released in 1.7.0 a week ago. Do you think we need it?
Oh and sorry for the title rename. I messed it up..
herndlm
changed the title
Make trait properties and methods first citizens of class nodes
Make trait properties and methods first class node cites
May 30, 2022
herndlm
changed the title
Make trait properties and methods first class node cites
Make trait properties and methods first class node citizens
May 30, 2022
I added a new virtual node ClassMethodNode and added isDeclaredInTrait to it and to ClassPropertyNode. In general I like that approach, but there are a couple of open questions for those two classes:
Currently I only need the name, private visibility yes/no and the trait info. But I already started duplicating other ClassMethod methods like isPublic, isStatic, .. how far should we go here? it's not complete yet either, e.g. getParams, getReturnType, .. are all still missing. I don't like the intermediate state
The duplicated methods are, well, duplicated. Would it make more sense to either store $originalNode or use it in the constructor to get the needed info? Or should as much as possible be injected via constructor?
I assume ClassPropertyNode grew a bit over time, e.g. it didn't have the $originalNode in it's initial state, whatever we do here, should we do this to ClassPropertyNode too then? Maybe for consistency reasons a bit of cleanup afterwards would be good :) but currently I don't fully understand where to go with those 2 classes exactly.
I guess my first intuition would be to kind of decorate a ClassMethod, meaning to hold one internally and support the same methods by passing everything through. Or just expose that internal node completely. But additionally add my custom method. Is this problematic memory-wise I assume?
UPDATE: I was so focused on ClassPropertyNode that I completely overlooked your other example and the fact that they don't have to be a real node.. Currently cleaning this up heavily :) And I guess ClassPropertyNode can't be cleaned-up easily, looks like it's in BC-lockdown mode
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is basically the refactoring I had in mind in #1362
The current state is already a bit weird IMO, in general trait props and methods were ignored when gathering class nodes. Then recently I added trait props to a dedicated array, just to use them also via uninitialized props checks. I did not add them to the normal props because that would lead to annoying new errors via
UnusedPrivatePropertyRule.Then just yesterday I added trait property promotion usage to the normal property usage, but ignoring other potential trait prop usages elsewhere. And trait methods are still completely ignored.
So what does this do different now?
Trait props and methods are gathered together with their class counterparts. And rules just use that. Additionally the information if the prop or method came from a trait is also remembered and only that is used to skip trait props / methods in those 2
UnusedPrivate*rules, that were annoying.I think in the long-run this makes more sense and avoids more weird trait problems. But maybe the details could be modified a bit. WDYT?