This repository has been archived by the owner on May 7, 2020. It is now read-only.
Exposing a Language Server #4148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kubawolanin would you mind having a look? My VSCode with openhab/openhab-vscode#31 (unchanged) connects successfully and also gets some validation complaints successfully from the server:
I'm not so sure though about the "content assist" completion proposals: they look exactly the same as before, but I'm not sure whether this is a flaw on runtime-side or if that's actually expected. |
|
Wow @SJKA this looks awesome! 😮
Current Completion provider gets a list of Item's completions directly from the REST response, see this file and the ItemsModel class. Can't emphasize enough how cool it is, man! 😎 |
|
Hey @SJKA, I'm trying to build your branch but there are tests that are failing: Click to expand build log for `mvn clean install` in `/smarthome/` directoryAny idea what I'm doing wrong? ;-) |
|
@kubawolanin I have these failing tests when doing local builds on my Mac also from time to time. Best if you simply use a |
kubawolanin
added a commit
to kubawolanin/smarthome
that referenced
this pull request
Aug 28, 2017
Thanks @kaikreuzer for [the info](eclipse-archived#4148 (comment))! :-) Signed-off-by: Kuba Wolanin <hi@kubawolanin.com> (github: kubawolanin)
|
FTR: I didn't update the (karaf) features yet - this time by intention because of eclipse/xtext-core#446 |
kaikreuzer
pushed a commit
that referenced
this pull request
Aug 31, 2017
Thanks @kaikreuzer for [the info](#4148 (comment))! :-) Signed-off-by: kubawolanin <hi@kubawolanin.com>
kaikreuzer
suggested changes
Sep 25, 2017
| for informational purposes only, and you should look to the Redistributor's license for | ||
| terms and conditions of use.</p> | ||
| <p> | ||
| <strong>Joda-Time 2.3</strong> <br/><br/> |
There was a problem hiding this comment.
Err, did you maybe choose a bad about.html to copy in here...?
| private void listen() { | ||
| try { | ||
| socket = new ServerSocket(PORT); | ||
| logger.info("Language Server running on port {}", PORT); |
There was a problem hiding this comment.
Maybe better "started" instead of "running"?
| private void handleConnection(final Socket client) { | ||
| logger.debug("Client {} connected", client.getRemoteSocketAddress()); | ||
| try { | ||
|
|
| Future<?> future = launcher.startListening(); | ||
| future.get(); | ||
| } catch (IOException e) { | ||
| logger.error("Error communicating with the LSP client", e); |
There was a problem hiding this comment.
This is likely an "external" issue like a client that closes the connection. If we want to log it at all, we at least should not include the stacktrace here as it is no bug on our end.
targetplatform/smarthome.target
Outdated
| @@ -101,6 +101,10 @@ | |||
| <repository location="http://download.eclipse.org/modeling/emf/emf/updates/2.11/core/R201506010402"/> | |||
| </location> | |||
| <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit"> | |||
| <unit id="org.eclipse.lsp4j.sdk.feature.group" version="0.2.0.v20170518-0647"/> | |||
There was a problem hiding this comment.
Is this an incubation project? If so, we might require a CQ for it, right?
There was a problem hiding this comment.
The due diligence process allows "Re-Use of Eclipse Project or Plug-in (Orbit excluded) or bundle that has undergone a release review". It doesn't say anything about incubation status of Eclipse projects. LSP4J has been formally released.
Did I miss anything?
There was a problem hiding this comment.
Ok, then we are lucky (and I had something wrong in mind... reminder to self: incubation!=unreleased).
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
sjsf
force-pushed
the
lsp
branch
2 times, most recently
from
b19978a
to
45ba3b5
Compare
kaikreuzer
reviewed
Sep 25, 2017
| @@ -89,7 +88,7 @@ private void handleConnection(final Socket client) { | |||
| Future<?> future = launcher.startListening(); | |||
| future.get(); | |||
| } catch (IOException e) { | |||
| logger.error("Error communicating with the LSP client", e); | |||
| logger.warn("Error communicating with the LSP client"); | |||
There was a problem hiding this comment.
a message or a client reference might be helpful, though, don't you think?
sjsf
force-pushed
the
lsp
branch
2 times, most recently
from
7e595ce
to
f5b94a1
Compare
|
I have updated the Karaf packaging to a state where it "builds on my machine". I hope that Travis comes to the same conclusion. Dear Karaf packaging experts, please have a close look if you can make some sense of my copy'n'paste in the |
maggu2810
reviewed
Sep 25, 2017
| @@ -24,6 +24,7 @@ | |||
| <feature dependency="true">esh-tp-jmdns</feature> | |||
| <feature dependency="true">esh-tp-paho</feature> | |||
| <feature dependency="true">esh-tp-xtext</feature> | |||
| <feature dependency="true">esh-tp-xtext-ide</feature> | |||
There was a problem hiding this comment.
Add also the lsp4j ESH TP feature.
|
|
||
| <feature name="esh-model-lsp" version="${project.version}"> | ||
| <requirement>esh.tp;filter:="(&(feature=xtext-ide)(version>=2.12.0)(!(version>=2.13.0)))"</requirement> | ||
| <feature dependency="true">esh-tp-lsp4j</feature> |
There was a problem hiding this comment.
Rely on esh-tp features using "requirement" and a filter expression as you have done in the line above for xtext-ide (first add your lsp4j optional feature dependency to the tp).
There was a problem hiding this comment.
awesome, I coudln't get the "requirement" statement to work - it seems like your above hint was the missing piece. THANKS!
Will do so, of course...
There was a problem hiding this comment.
Some background: Every solution could create its own target platform feature definition.
The esh-base feature depends on the esh-tp feature.
All features that are part of esh-tp are using dependency="true" so they are only considered if someone depends on it.
We are using the requirement declaration so we state which esh targetplafrom feature is necessary...
Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>
kaikreuzer
approved these changes
Sep 25, 2017
|
Travis seems to have problems this afternoon, it isn't at all starting the builds. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a first shot on the Language Server support so that we can jointly work on it.
The new bundle
o.e.sh.model.lspexposes a Language Server on (currently fixed) port 5007.relates to openhab/openhab-vscode#31
fixes #3898
Signed-off-by: Simon Kaufmann simon.kfm@googlemail.com