-
Notifications
You must be signed in to change notification settings - Fork 392
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
Consider token to limit the chains that are searched #2835
Consider token to limit the chains that are searched #2835
Conversation
@rgrunber @jdneo @testforstephen @snjeza what do you think about this change ? |
We could increase this, but as mentioned in #2730 (comment), I think we'll be limited by the performance of the chain search. Increasing the maximum chains might not necessarily mean more chains if we're always hitting the timeout, and returning no results. Part of the reason I kept it so small, is I felt there would be a limited number of chains that are useful. |
I will have a look at that root cause to see if we can improve the type resolution over there in jdt. |
ef32135
to
79642ad
Compare
@rgrunber Can you check my new way of tackling the same issue ? if it looks good, I can add the changes to jdt chain finder and use it in JDT LS, while we keep that open for JDT project adapt if there is any contributor for JDT.UI. |
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.
...e.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandlerChainTest.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/ChainFinder.java
Outdated
Show resolved
Hide resolved
@rgrunber WDYT about latest changes ? If this looks good, would you prefer adding this changes to jdt.ui or should we keep a copy of ChainFinder in jdt.ls and evolve further here ? |
c214002
to
a0d6d1e
Compare
Could we use |
Depends on eclipse-jdt/eclipse.jdt.ui#1282 |
a0d6d1e
to
db8b7c0
Compare
@rgrunber the PR is ready now |
@rgrunber do you think we can merge this as well to master ? |
Yes, I plan to review this. I'm guessing if we merge this, we can also close #2935 (that commit is included here) ? I guess my only complaint there was about using |
Yes you got my reasoning right @rgrunber. Let me know if you see any improvements that we should do, otherwise it would be nice to have both of this PRs in master :) |
15b0a01
to
a074ee8
Compare
...core/src/org/eclipse/jdt/ls/core/internal/contentassist/ChainCompletionProposalComputer.java
Outdated
Show resolved
Hide resolved
...core/src/org/eclipse/jdt/ls/core/internal/contentassist/ChainCompletionProposalComputer.java
Outdated
Show resolved
Hide resolved
...rc/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalReplacementProvider.java
Show resolved
Hide resolved
Also, there appear to be some test failures : https://ci.eclipse.org/ls/job/jdt-ls-pr/5176/ . The ones in CompletionHandlerChainTest seem consistent. |
@rgrunber did running unit tests method changed recently for JDT.LS tests ? When trying to run the unit tests I get the following error
|
f3466a4
to
4895d43
Compare
PDE definitely wouldn't be in the target platform of JDT-LS, so not sure why it would be needed. It should be part of the platform from which you're launching the plugin-in test though. Maybe you could try clearing any JUnit Plugin Test launch configurations ? |
@rgrunber I found that adding the following into the MANIFEST and target platform file in test module to resolve the issue.
|
@rgrunber also I found the reason we have the remove snippet text. Without it the tests are failing with following mismatch in Label assertions of the completions Expected: streams[i].toList() : List |
Provided a better fix for this |
This only needed to be added into target platform. |
@rgrunber the failing tests right now |
If you're using vscode-pde extension to run JUnit plugin test, the latest [email protected] has fixed this issue. |
6e0c134
to
447a11d
Compare
I tried installing, but that gave me some errors on conflicting modules
|
Where did you get the vscode-pde extension? Apparently the pde version you're using does not match what's included in the marketplace version. See https://github.com/testforstephen/vscode-pde/blob/795f86673ba29f50f7c1f3e09c63d9d155577bfc/package.json#L62, the dependency version it uses is |
Strange, i updated it from marketplace. I will try to clear any cached extension files and try installing from marketplace again. |
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.
Just noticed you reverted the target changes here. Ok, this was exactly what I was waiting for. We're aiming for a release by Thursday, but once that's done, I'll just merge this in.
Sure i can make it ready by resolving the conflict as well. |
- simplify the code by reusing CompletionProposalRequestor to create items - add support for javadoc for chain completions
…nt proposals are shown.
447a11d
to
bf27d6a
Compare
@rgrunber resolved the conflicts |
This draft use a copy of ChainFinder to tryout using token to limit the number of chain produced by the the chain finder. Instead of increasing the chain size which could potentially go into more deeper search, this approach provides the capability to user to filter by the token to find the most relevant chains the user is searching for.