-
Couldn't load subscription status.
- Fork 187
Autocomplete speedup vol. 2 #599
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
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 my_user_defined_fu| -> \my_user_defined_function, but not for array_mer| -> \array_merge.
If so, it should probably be expressed as a textEdit rather than insertText, since at least lsp-mode in Emacs does not know what to do in the case where the insert text does not match the text that has been written.
Currently when in a namespace we complete to \array_merge afaik. If we had settings there probably should be a setting for it.
Need some more time to review this.
Would you mind updating the description?
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
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
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!
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) === '()') {
pkirkinezis
commented
May 11, 2018
Hello
Do we have an estimation date on when this will be merged if it will be merged ?
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:
git remote add Declspeck git@github.com:Declspeck/php-language-server.git
Fetch it:
git fetch Declspeck
Check it out:
git checkout autocomplet-speedup
Merge in master (make sure master is up to date):
git merge master
Fix merge conflicts and commit
Push to your own fork:
git push -u jens1o autocomplet-speedup
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 📦🚀
Uh oh!
There was an error while loading. Please reload this page.
This PR does everything that the original Autocomplete Speedup (#451) does, and what your branch of it did:
Indexinto a tree structure, rather than an array of Fqns to definitions.Indexto speed up::,->completions.In addition to that, this PR
Indexto 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:
CompletionTestCompletionProvider.phpto make it passcomposer lint