-
Notifications
You must be signed in to change notification settings - Fork 149
feat: add Beta support for Inline Completion Request #886
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
base: main
Are you sure you want to change the base?
feat: add Beta support for Inline Completion Request #886
Conversation
Adds the inline completion request as proposed for LSP 3.18 (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.18/specification/#textDocument_inlineCompletion). I have added the Beta annotation as suggested in #856 but this makes tests fail because this dependency is not allowed. There are also some problems with the javadoc which I have not yet figured out. If you are interested in the PR, I would clean this up. Are you interested in the PR? In that case, since this is my first contribution to the project, do you see any problem in how I translated the spec to the Xtend file? Thanks |
Thanks so much @rubenporras to work on this issue. LSP4IJ (LSP support for IntelliJ) is super interested with that, we wait for this support redhat-developer/lsp4ij#643 |
a7da334
to
811ff81
Compare
811ff81
to
b196f8b
Compare
@rubenporras Congratulations on your first contribution! It is really appreciated. I have already started a review. |
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.
@rubenporras I have completed the first round of review. If possible, please address review comments with a separate commit. Thank you for your time and effort!
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/services/TextDocumentService.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
@pisv, thanks a lot for the thorough review. I have addressed all the comment but the ones about indentation being a bit off because I could not figure it out. I have also fixed the errors reported in the javadoc when running the gradle build. I still need advice on how to fix the problem with the Beta annotation. At the moment the annotation makes the Lsp4jArchitectureTest.testNoDependenyToGuava fail. |
I have been looking at Lsp4jArchitectureTest.testNoDependenyToGuava, and my proposal would be that to avoid the dependency we can declare our own Beta annotation in a new package org.eclipse.lsp4j.annotations. That should be straightforward. |
+1 for declaring our own |
@rubenporras Unfortunately, I will not be able to make another round of thorough review until next week. But rest assured that your work is much appreciated! Thank you. |
Thanks for the input. I have added another commit on top with an own Beta annotation. I will also not be available until next Tuesday. I guess you will enjoy the Easter time the same as I :). |
Thanks for the input. I have added another commit on top with an own Beta annotation. I will also not be available until next Tuesday. I guess you will enjoy the Easter time the same as I will :). |
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/InlineCompletionTriggerKind.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/services/TextDocumentService.java
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/services/TextDocumentService.java
Outdated
Show resolved
Hide resolved
org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend
Outdated
Show resolved
Hide resolved
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.
@jonahgraham I'd like to solicit your input on this idea of having our own Beta
annotation, and its proposed implementation. In its current form it has been copied from Guava. Does it necessitate a separate IP review for this PR? In that case, I'd probably prefer to define a similar annotation from scratch.
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.
Yes, this is correct.
I have just removed the @GwtCompatible
annotation and adapted the copyright to add and othres.
I could start from zero if needed, but other than having a different javadoc, there is not much room for deviating.
No description provided.