Complete inside class constant declaration #1985
Conversation
|
I'm not sure if it's the correct logic but first tests suggest that it works. |
| '<?php class Foobar { private const FOO; private const BAR = self::F<> }', [ | ||
| [ | ||
| 'type' => Suggestion::TYPE_CONSTANT, | ||
| 'name' => 'self::FOO', |
There was a problem hiding this comment.
Please check if the expectation is correct.
There was a problem hiding this comment.
why do you suspect it not to be correct?
There was a problem hiding this comment.
At the first I started with name => 'FOO' - the whole 'self::FOO' caught my attention.
There was a problem hiding this comment.
hmm, yeah that doesn't seem quite right, did you test it in the editor?
There was a problem hiding this comment.
Yes,
complete-in-class-declaration.mp4
but I found that the new test case checks something that works already on master 😁 so I need to add the proper case before refactoring discussed in the other comment.
There was a problem hiding this comment.
it is an artificial suggestion:
| @@ -42,6 +43,10 @@ public static function expression(?Node $node): bool | |||
| return false; | |||
| } | |||
|
|
|||
| if ($parent instanceof ConstElement) { | |||
There was a problem hiding this comment.
think we can move this to the similar expressions in the final return below?
There was a problem hiding this comment.
or is it triggered by classMembersBody somehow?
There was a problem hiding this comment.
The final return is unavailable in this case because of classMembersBody:
Maybe moving the new condition into classMembersBody() would be good idea but
- I need to ensure if we expect
trueif the parent of$nodeisConstElement - it's public and used in other places so we need to inspect what will happen then (with the hope that existing tests tests cover enough amount of cases first).
|
It's still not what I need: |
|
What are you missing ? |
|
Note that it does not work inside array. I need some free time to fix it again. |
|
it's a mine field, good luck! |
complete-in-class-declaration-2.mp4Now it's time to return to the question where |
|
I think it's fine, its not a "query" but just an I've just spent the past 30m trying to stop
but I think we can merge this in it's current state as an improvement? |
|
I agree that at the moment almost accurate suggestion list is better than none and we could merge this PR. Independently it would be good to keep in |
|
thanks,I updated the changelog and changed the test to simulate something more realistic... |
Closes #1978