Local LSP server that improves functionality like item completion

[SamuelBrucksch]

Initial LSP impl with example from MS sample LSP:
https://github.com/Microsoft/vscode-extension-samples/tree/master/lsp-sample

To test:

npm install
npm run compile
press f5 in vscode

Open a new file, and then select openhab as language or save as *.items/rules/...

This will fix #95 when all the work is done.

Signed-off-by: Samuel Brucksch sasliga@freenet.de

[SamuelBrucksch]

Unfortunately i did not find an easy way to use our local LSP as proxy for the remote LSP without reimplementing the whole LSP protocol. The vscode-languageclient can not run within the server because it requires a module, that can only be used in the client:
https://github.com/Microsoft/vscode-languageserver-node-example/issues/36

My idea was something like this:
vscode extension -> local lsp -> client in local lsp -> remote lsp
However as the languageclient can not run in the server part (see issue from above) i cant follow that idea.

Instead i now run local and remote LSP server in parallel. In this screenshot you can see diagnostics from local and remote LSP. The green underlined part is from local LSP because its all uppercase (just a sample, no real diagnostics yet) and the red underlined parts are from openhab remote LSP:

So we still can implement the item completions in out local LSP server, add diagnostics to the one we ge from the remote server and also do all the other stuff in our local LSP like jumpt to reference and so on...

Its not as nice as extending the remote server, but its still better than nothing.

Maybe later when i have more time i can look into how to write an own language client, but for now this should be enough.

[MHerbst]

I will have a closer look at the implementation at the end of the week. It will also be necessary to do some modifications in the remote LSP because it currently announces to many capabilities which leads to context menu function that won't work.

[SamuelBrucksch]

I added items completion now. Its just a proof of concept and i'm not sure if it is very stable. However now we get the items from rest in the beginning, cache them and update them based on SSE. So we also get new items and items that are removed.

Will clean up the code and make it a bit nicer. I'm also open for ideas on how to improve the ItemsCompletion code in the server part.

[SamuelBrucksch]

I think i'm currently at a stage that reflects the functionality from before but with much faster completion.

I can either continue on implementing LSP features, or wait for a review and merge and then add new PRs for new features, which would make more sense for working simultaniously on seperate features.

[SamuelBrucksch]

Maybe some remarks on what i did:

1. i seperated lsp server (server) and extension (client)
2. extension config is in root package json, {server,client}/package.json only contains dependencies and some meta info
3. slightly adapted npm scripts, now we have npm run compile that only compiles and npm watch that watches for changes. compilation is done from the project root and not for each module.
4. started a local and a remote language client to connect to both local and remote LSP server
5. Added LSP impl
6. added completion items provider that caches the items and updates them with SSE
7. added validation provider that validates documents, this is still WIP and does not do anything yet. However its a preparation for future features.

I think thats all. Maybe we need a utils package for code that is used in both modules like IItem.ts and Item.ts. Maybe more will follow.

[SamuelBrucksch]

Should i add changelog entries already or is this done seperately? The current changelog would look something like this:

0.5.0 -TBD

• Added local LSP server (Local LSP server that improves functionality like item completion #122)
• Fixed sorting order in items explorer (Sort order in items explorer not working anymore #125)
• removed settings param 'restCompletions'
• renamed setting param 'lspEnabled' to 'remoteLspEnabled'
• renamed setting param 'lspPort' to 'remoteLspPort'

[MHerbst]

There seems to be a problem with outline view. With the test vsix (#122 (comment)) I did not see any outline. After I went back to the marketplace version the outline was OK.

[SamuelBrucksch]

Outline works for me (checked in sitemap, rules, items). Seems like outline items are provided by remote LSP so make sure it is enabled to see the outline. At least when i disabled remote lsp no outline was there, but when i enabled it, it was there...

[SamuelBrucksch]

Latest vsix:
openhab-0.4.1.zip

[SamuelBrucksch]

Any more comments or reviews?

[SamuelBrucksch]

Come on guys, this is important to work on other tasks as well.

[MHerbst]

@SamuelBrucksch @Confectrician My runtime tests were OK. Did not find any problems.
Just one idea (if possible): the context menu for code completion gets all item changes via SSE. But the item view must be manually updated. Would be nice if this could be updated automatically as well.

[SamuelBrucksch]

Hi, about the items view: i noticed that as well. I think we can also use the completion items from LSP for that. Behaviour did not change, it was like that from the start so i wanted to do this in a seperate PR later. Not sure ifwe actually need the values in completions at all, i thought about a hover provider that displays the values on hover of an item in code.

[MHerbst]

Not sure ifwe actually need the values in completions at all, i thought about a hover provider that displays the values on hover of an item in code.

In completions completion the item names and an information about the type are sufficient.
Hover with current value would be nice :-)

[Confectrician]

Hi all,

I finally had some time for (functional) testing and could not recognize any missbehavior for now.
I didn't look at the code yet in detail, but will do so at least for the ts parts.

Sorry for the delay, but we are all doing this in our free time besides work/school/study/whatever.
I know it can be annoying sometimes, but that's one of the compromises open source can bring.

[Confectrician]

@SamuelBrucksch maybe you can squash your commits and add a sign off now?

[Confectrician]

Also https://app.codacy.com/app/Confectrician/openhab-vscode/pullRequest?prid=2701365 shows some issues.
All of them are located in the js server part and most of them are about quoting style.
Maybe you could have a look at that.
But since it doesnt reduce functionality for now thats not a must have for me.

Edit:
Looking at our .ts files they seem to have single quote used most/all times,
so it would only be consistent to leave them in js single quoted too for now. :)

[SamuelBrucksch]

Hi, whats the best way to squash them? I saw rebase -i works or reset soft and then merge --squash, but i guess all of them would result in a git push -f at the end so all the single commits are gone, is that right?

Double quotes for JS does not make much sense as there are also other ways to create strings like

const number = 5
console.log(`Number is ${number}`)

This does not work with double quotes. Also its easier to use quotings and so on in single quotes without disturbing escape characters.

[Confectrician]

so all thesingle commits are gone

Yep but this doesn't matter in this state.
We are doing squash and merge in openhab repos anyway.
So at the latest when merging, this will be one commit in master.

We can leave it too, its just some cosmeticcal stuff to satisfy pullapprove . 🙂

[Confectrician]

@MHerbst or @kubawolanin any additional comments?
We can change stuff anyway and from my tests its functional, so i would say lgtm.

[Confectrician]

[kubawolanin]

These are minor things but since we're using version control, let's avoid checking in a commented out code

[SamuelBrucksch]

Sure, i just left it there so we can see faster how it works when we enhance LSP. But i can remove it, no problem.

[Confectrician]

I would remove them too.
Those are just empty functions.

We could open an issue instead, which reminds us that these functions (names written down) could be used later.

[SamuelBrucksch]

I removed the commented code. Do you still want me to squash everything even if is squashed anyways?

[kubawolanin]

I removed the commented code.

Perfect, thanks!

Do you still want me to squash everything even if is squashed anyways?

I think we're good here, since you've signed off your first comment and committed all code with one git user :-)
Let's get this on master now!

Thank you for your work.

[SamuelBrucksch]

Yay! Thanks for reviewing. Now i will add a lot more PRs ;).