Add multi-server feature #85
Conversation
…on description text
- replace all <'key> instances with their dedicated 'defconst' - distinguish between 'server top-node and 'server key
…t-server-image-name' functions, improve error message
Multi server feature
…'lsp-docker-get-server-image-name' functions, improve error message"
|
@sfavazza , I will take a look in a few days, a bit busy right now :) |
|
The feature is there it won't go anywhere 😊 |
README.org
Outdated
| @@ -143,6 +147,7 @@ | |||
| It is structured in the following way: | |||
|
|
|||
| #+begin_src yaml | |||
| # signle server configuration | |||
There was a problem hiding this comment.
Addressed in b23d606
|
@sfavazza , I finally managed to get a bit of time to review all changes. First of all I have to say that you've done a wonderful work with not only functional changes but refactoring as well! As for the changes everything looks good, I think you can merge this PR (but fix this small typo first though :) @yyoncho , sorry to bother, could you please take a look at these changes? I don't feel comfortable reviewing this amount while not being a elisp guru myself :) |
|
@factyy thank you for dedicating the time to this review, really appreciated. Please observe that I don't have the necessary rights to merge, so let's wait for the feedback of @yyoncho and then you guys can merge (if everything is proper). Unless you already did it, I strongly suggest to try these changes also on your machine via the simple setup suggested in the |
| @@ -300,63 +336,67 @@ the docker container to run the language server." | |||
| (defun lsp-docker-get-config-from-lsp () | |||
| "Get the LSP configuration based on a project-local configuration (using lsp-mode)" | |||
| (let ((project-config-file-path (lsp-docker--find-project-config-file-from-lsp))) | |||
| (or (lsp-docker-get-config-from-project-config-file project-config-file-path) | |||
| (ht-copy lsp-docker-persistent-default-config)))) | |||
| (if project-config-file-path | |||
There was a problem hiding this comment.
@sfavazza , I noticed a small issue: what if there is no config file? We have to use the default one, but here we are getting an error instead
|
Managed to test it briefly, everything looks fine, thanks @sfavazza |
High level summary of changes: