-
Notifications
You must be signed in to change notification settings - Fork 185
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
Autocomplete speedup vol. 2 #599
Conversation
This reverts commit 48bbbb5.
I got another (unrelated) question about the completion logic - for roamed symbols, do we want to complete the name as fully qualified? It seems to make sense for If so, it should probably be expressed as a |
Currently when in a namespace we complete to |
Need some more time to review this. |
Sure, I'll update it after work today (or at the very latest tomorrow) - I already forgot what I'm wanting to merge here, so I have to take another look myself. It looks like I also have a conflict to resolve. |
...And thanks for reviewing the other PRs! |
I updated the description and merged master into this. |
src/CompletionProvider.php
Outdated
foreach ($list->items as $item) { | ||
// Remove () | ||
if (is_string($item->insertText) && substr($item->insertText, strlen($item->insertText) - 2) === '()') { | ||
$item->insertText = substr($item->insertText, 0, strlen($item->insertText) - 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to use strlen
here(this line and the line before).
Use substr($item->insertText, -2);
(or substr($item->insertText, 0, -2);
) instead.
src/Index/Index.php
Outdated
@@ -216,10 +280,164 @@ public function unserialize($serialized) | |||
public function serialize() | |||
{ | |||
return serialize([ | |||
'definitions' => $this->definitions, | |||
'definitions' => iterator_to_array($this->getDefinitions(), true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the second parameter is already the default one, no need for the true
.
Thanks for the feedback @jens1o - I finally got around to addressing it. |
Thank you! |
src/CompletionProvider.php
Outdated
@@ -341,7 +341,7 @@ public function provideCompletion( | |||
foreach ($list->items as $item) { | |||
// Remove () | |||
if (is_string($item->insertText) && substr($item->insertText, strlen($item->insertText) - 2) === '()') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line can be simplified too
if (is_string($item->insertText) && substr($item->insertText, -2) === '()') {
Hello |
Short answer, no, we don't have an estimate date. |
Hey @Declspeck, sorry I had forgotten about this PR. Just took a look and it looks good to me. Do you think this is in a mergeable state / are you still interested in getting it across the finish line? |
@felixfbecker I'd be interested in completing this PR. What do I need to do? Copy the clone? |
Awesome. Here is roughly what you need to do: Add the fork as a git remote:
Fetch it:
Check it out:
Merge in master (make sure master is up to date):
Fix merge conflicts and commit
|
Awesome, thanks for the heads up! |
🎉 This issue has been resolved in version 5.4.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR does everything that the original Autocomplete Speedup (#451) does, and what your branch of it did:
Index
into a tree structure, rather than an array of Fqns to definitions.Index
to speed up::
,->
completions.In addition to that, this PR
Index
to speed up completion on names, e.g.array_mer|
and adds benchmark for that.TestCla|
should not completeOtherClass
)use Microsof\PhpParser\Node; Node\Stat|
correctly, adds tests for thisTestNamesp|
no longer completesTestNamespace\SubNamespace\Foo\Bar
.Misc. unrelated changes:
CompletionTest
CompletionProvider.php
to make it passcomposer lint