Signature help #547
Signature help #547
Conversation
Codecov Report
@@ Coverage Diff @@
## master #547 +/- ##
============================================
+ Coverage 79.36% 80.86% +1.49%
- Complexity 836 877 +41
============================================
Files 56 61 +5
Lines 1900 2028 +128
============================================
+ Hits 1508 1640 +132
+ Misses 392 388 -4
|
src/SignatureHelpProvider.php
Outdated
| : 0; | ||
|
|
||
| // Get information from the item being called to build the signature information | ||
| $calledDoc = $this->documentLoader->getOrLoad($def->symbolInformation->location->uri)->wait(); |
There was a problem hiding this comment.
Never use wait except in tests, it blocks the event loop. Make it a coroutine and yield the Promise
There was a problem hiding this comment.
Thanks, I'm still getting my head around this event loop business. I've updated this to wrap this function in a coroutine.
src/SignatureHelpProvider.php
Outdated
| ++$i; | ||
| } | ||
| } | ||
| if (is_null($found)) { |
| ]; | ||
| } | ||
|
|
||
| private function createSignatureHelp(array $info): SignatureHelp |
There was a problem hiding this comment.
I would prefer to add constructors to SignatureHelp etc. so we don’t need this factory here
There was a problem hiding this comment.
I've added constructors and utilised them (I actually found this style of OO with public members a bit strange but just copied the existing style). I still like to have this creator as otherwise we end up constructing lots of objects in the data providers and it becomes harder to maintain - if a constructor changed there would be a lot of rework.
There was a problem hiding this comment.
Please use the constructors directly - especially after your PR, you can get signature help for the constructor parameters, but you can't for the array hashes ;)
That's how it's done in all the other tests too.
There was a problem hiding this comment.
There is no reason to make the members private and write getters and setters for them as they represent the JSON that gets sent over the wire (which only has public properties). They are generally dumb value objects that don't contain any complex state that needs to be kept private.
There was a problem hiding this comment.
Got it, makes sense - updated now, thanks
|
@felixfbecker I have updated this PR to store signatureInformation in the index |
|
Awesome work! |
This is an alternative to the existing signature help PR, which I didn't realise existed when I initially did this work. Since it doesn't seem to quite be there, I thought I would raise this as I am keen to see signatureHelp be part of php-language-server.
I would have helped out with the work on the other branch if I knew it existed but I didn't, so this is an alternative implementation. I believe it has a few of benefits:
Here is an example of it in action:
I am open to suggestions on how to better display the signature help (params/docs/anythign else)