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]

Looks like sign off did not work because i commited from two different PCs with different git settings. Looks like i have to do it again because 17 commits are with wrong sign off -.-

@Confectrician You want me to create a new PR with correct sign off? However then the many commits will be lost and it will be one big commit that is correctly signed.

[Confectrician]

No, no need to do that. Pullapprove is just a tool at the end, which can be a help but shouldn't prevent us from thinking. I can see that there are sign off statements with valid data.

[Confectrician]

Also please note that this is a huge change and it will take some time for testing it.

[SamuelBrucksch]

Hey,

i have some time during the holidays to work a bit more on this or other related projects, so it would be nice if you could look over this so i can continue with other issues or features or with improving this impl.

BR

Samuel

[Confectrician]

I can't promise anything. It is the last working week of the year and I am still busy there. Maybe at the weekend.

@MHerbst did you have time already?

[MHerbst]

Unfortunately not, but tomorrow is my last work day for this year. This means that I can start with testing (and reviewing) in two or three days.

[SamuelBrucksch]

I would like to write some tests for the LSP, however TS is actually not really my thing, i'm more into pure JS. Would it be ok for you/the project, if the LSP would be written in pure JS? My experience there is much better and for me its much easier to write tests then instead of experimenting with TS. I would be much faster that way. Or is it really necessary to use TS? Its converted into JS anyways.

[kubawolanin]

Hey @SamuelBrucksch, Thank you for your contributions. I've been absent from any development activities for a long time here - had to take care of my health. Kudos to @Confectrician and @MHerbst for taking care of this project.

Would it be ok for you/the project, if the LSP would be written in pure JS?

I think it's ok for an initial contribution, yes. We can then think of migrating to TS later, so both projects are aligned in terms of code style.

And tests are always welcome :-) I promised myself to cover crucial parts of the code with them long ago. Sadly, didn't find time for them later on.

[SamuelBrucksch]

Hi,

thanks for your message. I now added a pure JS impl with Jest tests. Current coverage looks like this:

----------------------------|----------|----------|----------|----------|-------------------|
File                        |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------------------|----------|----------|----------|----------|-------------------|
All files                   |    93.98 |    82.61 |    85.71 |    93.65 |                   |
 src                        |      100 |      100 |      100 |      100 |                   |
  LSPServer.js              |      100 |      100 |      100 |      100 |                   |
  Server.js                 |      100 |      100 |      100 |      100 |                   |
 src/DocumentValidation     |      100 |      100 |      100 |      100 |                   |
  DocumentValidator.js      |      100 |      100 |      100 |      100 |                   |
 src/ItemCompletion         |    90.36 |    78.95 |    76.67 |    90.36 |                   |
  Item.js                   |    63.16 |     12.5 |       60 |    63.16 |... 0,71,82,88,107 |
  ItemCompletionProvider.js |    98.44 |    96.67 |    93.33 |    98.44 |                36 |
----------------------------|----------|----------|----------|----------|-------------------|
Jest: "global" coverage threshold for branches (90%) not met: 82.61%
Jest: "global" coverage threshold for functions (100%) not met: 85.71%

Still some tests missing. But overall quite good already.

I guess it makes sense to remove the TS impl for now if i start with the JS impl as it will be too confusing to review both, right?

If it were my decision i would also go for JS in the extension, but thats just my personal preference as i'm more used to it.

[SamuelBrucksch]

I think i have all the tests in place more or less...

Unit Coverage:

----------------------------|----------|----------|----------|----------|-------------------|
File                        |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------------------|----------|----------|----------|----------|-------------------|
All files                   |      100 |    95.74 |      100 |      100 |                   |
 src                        |      100 |      100 |      100 |      100 |                   |
  LSPServer.js              |      100 |      100 |      100 |      100 |                   |
  Server.js                 |      100 |      100 |      100 |      100 |                   |
 src/DocumentValidation     |      100 |      100 |      100 |      100 |                   |
  DocumentValidator.js      |      100 |      100 |      100 |      100 |                   |
 src/ItemCompletion         |      100 |    94.87 |      100 |      100 |                   |
  Item.js                   |      100 |     87.5 |      100 |      100 |                56 |
  ItemCompletionProvider.js |      100 |    96.77 |      100 |      100 |                31 |
----------------------------|----------|----------|----------|----------|-------------------|

Integration Coverage:

----------------------------|----------|----------|----------|----------|-------------------|
File                        |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------------------------|----------|----------|----------|----------|-------------------|
All files                   |     62.5 |    51.06 |    53.06 |    65.89 |                   |
 src                        |    57.45 |       25 |    27.78 |     67.5 |                   |
  LSPServer.js              |      100 |      100 |      100 |      100 |                   |
  Server.js                 |    53.49 |       25 |    27.78 |    63.89 |... 14,130,139,142 |
 src/DocumentValidation     |      100 |      100 |      100 |      100 |                   |
  DocumentValidator.js      |      100 |      100 |      100 |      100 |                   |
 src/ItemCompletion         |    63.53 |    56.41 |    66.67 |    63.53 |                   |
  Item.js                   |    52.63 |       25 |    46.67 |    52.63 |... 0,71,82,88,107 |
  ItemCompletionProvider.js |    66.67 |    64.52 |    86.67 |    66.67 |... 8,89,90,92,167 |
----------------------------|----------|----------|----------|----------|-------------------|

Integration could still be a bit better, for example i have one TODO still left in the code, to create, update, remove items via rest api and check if the events come in. However that is a quite complex test and i will do it later.

[SamuelBrucksch]

As this is quite a huge change i could also offer a Pair Review via skype or whatever to explain what i did.

[SamuelBrucksch]

The config property 'openhab.restCompletions' might be obsolete now, as currently we always use rest completion. If the rest api is not available, then no items are proposed, but in all other cases we will have items completion except when 'openhab.lspEnabled' is set to false.

Is there still a need to disable rest completions? If yes we need to be able to turn it on and off in the local LSP server.

[MHerbst]

I was able to clone the sources and create start a debug session with the new code.

@SamuelBrucksch Maybe you could also create a vsix package. That would make testing easier. I think it would also be a good idea if you could write down shortly which features are now implemented in the local LSP and should work faster then before. This PR has so many commits that I have lost track of them.

I would then concentrate on testing the extension. Javascript is not really one of my preferred languages. TypeScript is OK for me, but I don't have enough experience or knowledge to review JavaScript code.

Is there still a need to disable rest completions? If yes we need to be able to turn it on and off in the local LSP server.

In case of installations with a large number of items, the REST calls may slow down everything. It then may help to disable the REST calls.

[SamuelBrucksch]

I was able to clone the sources and create start a debug session with the new code.

@SamuelBrucksch Maybe you could also create a vsix package. That would make testing easier. I think it would also be a good idea if you could write down shortly which features are now implemented in the local LSP and should work faster then before. This PR has so many commits that I have lost track of them.

You can build the vsix yourself now. there is an npm script 'package' which you can run with 'npm run package'. You have to install vsce before with 'npm install vsce -g'

Its still the change list from here: https://github.com/openhab/openhab-vscode/pull/122#issuecomment-446199834

I only added tests and changed from TS to JS after that.

I would then concentrate on testing the extension. Javascript is not really one of my preferred languages. TypeScript is OK for me, but I don't have enough experience or knowledge to review JavaScript code.

I tried to make the implementation clean, but maybe i did not achieve it everywhere. I used mainly JS classes so its easier to understand for Java developers. the tests and mocks look awful, i know. Its hard to understand if you never did it with jest. So maybe try to focus on the impl, that should be easy to understand. At least the logic.

Is there still a need to disable rest completions? If yes we need to be able to turn it on and off in the local LSP server.

In case of installations with a large number of items, the REST calls may slow down everything. It then may help to disable the REST calls.

REST items are now cached locally, so even if REST api is slow you will not notice because for completions the local cache is used. Thats why i asked if that property still makes sense. Even a list of 1000+ items should not be a problem.

[SamuelBrucksch]

Here is a vsix for testing:

openhab-0.4.1.zip

[MHerbst]

@SamuelBrucksch The item list is no longer alphabetically sorted and the items appear in a random order.

[SamuelBrucksch]

For me it is still ordered. However code snippets are seperated from completion items and small and capital letters in the beginning of the word are also seperated:

gGroup1 gGroup2 AutoHeating

Was this different before?

[MHerbst]

It is a bit strange. A problem with the sort order was first reported here: https://community.openhab.org/t/alphabetical-sorting-of-item-list-is-no-longer-possible-in-vscode-extension/61131 . I have then tried it with the version from the Marketplace and the sort order was OK. Then I installed the version you have attached here and now the sort order looks a bit strange:

I am not sure whether it was really introduced by your latest modifications but we need to make sure that we present the items in alphabetical order. For code completion in the editor the list is sorted correctly.

[SamuelBrucksch]

Ok thats the items list not the code completion.

Looks like this PR had some side effects noone noticed: https://github.com/openhab/openhab-vscode/pull/114

I assumed this was only used in completion. For me the order in the items list was never ordered alphabetically. Current version from vscode marketplace gives me something like this:

And it already contains the above mentioned PR. So i guess the sort order should already not work then.

Will see what happens if i put back the sort. Let me check...

[SamuelBrucksch]

Indeed it does not work anymore, i opened a seperate issue: https://github.com/openhab/openhab-vscode/issues/125

The sort order change should be a quick one, however i would propose to first merge this, so we do not get too many merge conflicts in here. If you want the other one merged first however i can take care of it and then merge it in here or so.

[SamuelBrucksch]

Here are some open points we should still discuss so i can finish it:

1. Configuration
   • should we differentiate between local and remote lsp in config?
   • openhab.lspEnabled used for both or use two settings instead?
   • openhab.restCompletions this is now in the local LSP and does not slow down code completion anymore, so should we keep this or remove it?

As this is a new feature, it might make sense to introduce new settings, so it is not disabled by all the people that had problems and then they will not see, that it works better now.

So for example openhab.lspEnabled is only used for remote LSP server and openhab.restCompletions is renamed to openhab.localLsp and deals with completion and other stuff. I dont think we need to have a config for each LSP feature as long as the implementation does work.

Or do you think it is better to have settings like

openhab.completion openhab.validation openhab.hover

and so on... However as long as the remote LSP offers more features than it provides, those will still be available as we do not have any controls over them.

[kubawolanin]

Hi Samuel,

should we differentiate between local and remote lsp in config?

That really depends on what config properties do you plan to incorporate for the local and if they are cannot be applied for the remote.

openhab.lspEnabled used for both or use two settings instead?

AFAIK we should keep the local language server online without a config flag turning it off. Most VSCode extensions operate that way. Thus, I would suggest renaming lspEnabled to remoteLspEnabled or a like.

openhab.restCompletions this is now in the local LSP and does not slow down code completion anymore, so should we keep this or remove it?

We can remove it, thanks :)

[SamuelBrucksch]

Hm, looks like my rebase added some more commits than were necessary... i did 'git rebase origin-remote/master' was that wrong?

Guess i need to check out how to rebase correctly...

Sorry for force pushing here...

[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 (#122)
• Fixed sorting order in items explorer (#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 (https://github.com/openhab/openhab-vscode/pull/122#issuecomment-449644967) 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]

:shipit:

[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 ;).