-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add multiple language server association with syntax elements #496
base: main
Are you sure you want to change the base?
Conversation
@@ -12,20 +12,20 @@ jobs: | |||
steps: | |||
- name: Install packages | |||
run: | | |||
sudo apt update | |||
sudo apt-get update |
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.
seems unnecessary?
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.
This is a minor fix. Using the apt command in scripts is discouraged, and it is recommended to use apt-get instead. According to the apt manual:
The apt(8) commandline is designed as an end-user tool and its behavior might change between versions. Although it aims to maintain backward compatibility, this is not guaranteed if a modification appears advantageous for interactive use.
That's why a warning message is issued when apt is employed in a script.
However, this fix can be safely ignored; nevertheless, there isn't a reason to ignore it.
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.
it has no pipeline here, maybe it's ok.
autoload/lsp/buffer.vim
Outdated
@@ -170,6 +176,7 @@ export def CurbufGetServerChecked(feature: string = null_string): dict<any> | |||
return {} | |||
endif | |||
|
|||
|
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.
redundant blank line.
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.
i saw your commit, as i knew yegappan would not squash the commits when merging, maybe you need to squash it later, otherwise you would see the commits history (and tho the contribute count too) a little redundant too, 😄
// just an irrelevant comment, and it's on yegappan.
filetype: ['javascript', ''typescript', 'graphql'], | ||
path: 'graphql-lsp', | ||
args: ['server', '-m', 'stream'], | ||
syntaxAssociatedLSP: ['graphqlTemplateString'], |
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.
how reliable of this? and need to dyn change/config the syntax value?
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.
I don't foresee a situation where we need to dynamically change the syntax. Can you provide an example to help me understand what you mean?
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.
i meant the syntax value was not reliable, or dynamic, perhaps if there was a way to auto detect the syntax value (which it belonging) would be great, otherwise e.g markdown file, there perhaps embedded some unconcern code block.
.github/workflows/unitests.yml
Outdated
- name: Prepare Tests | ||
run: | | ||
# install deps for the dummy lsp | ||
cd ./test/dummy-lsp/ | ||
npm install | ||
cd - |
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.
perhaps just using markdown lsp? vs/but not need to make up a dummy lsp.
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.
I merely wanted a simple LSP setup for a very basic use case, keeping the unit tests equally simple.
The dummy LSP can be configured to return varying responses to hover requests, thus serving as a convenient way to mimic different LSPs using just one. Without this approach, you would have had to rely on two distinct LSP implementations—for example, starting with a PHP file, configuring both phpactor
and html-languageserver
, and checking whether the correct LSP responds upon querying an htmlTag
. I find this complicates the tests unnecessarily.
Should implementing a fake LSP for simple mocking in tests prove problematic, replacing it with actual LSP instances wouldn't be an issue.
Do note that the dummy LSP could potentially be expanded for conducting additional tests. This idea serves as a suggestion rather than a requirement.
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.
you can use a markdown file which embedded clang/golang etc code block to test, which those had been config into test of this codebase, if so, the code base would be clean and easy.
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.
I modified the unit tests according to recommendations, removing the dummy LSP. Although these tests aren't as extensive, they ensure the right LSP is invoked, which should be enough. Testing real LSP execution isn't feasible as clangd doesn't understand Markdown, and ultimately, testing real LSP execution turns out to be unnecessary because selecting the appropriate LSP suggests that everything else will most likely fall into place.
thanks, seems a good try, BTW: truly there now still some problem so far no good answer, e.g:
|
With this proposal, I do not claim to fully address the complex challenge of multi-LSP, but rather aim to enhance existing options with a simple solution built upon Vim's inherent syntax identification capabilities. This works smoothly in cases where the LSP is designed to operate within the given context. As demonstrated, the proposed solution excels in cases like PHP files with embedded HTML or TypeScript files featuring GraphQL templates. By maintaining the primary LSP as the default and placing it first in the LSP list, complementary LSPs are included only when encountered within a specific syntax group. Notably, the script processes the complete syntax stack of the current word, starting from the highest level. Admittedly, cases similar to the YAML Ansible scenario cannot be resolved solely through the suggested approach, as decisions concerning the appropriate LSP cannot depend exclusively on file syntax. In such circumstances, tradeoffs must be made, leaning on other configuration options like call LspAddServer([
\{
\ 'name': 'angular',
\ 'filetype': ['html'],
\ 'path': g:lib_path . 'node_modules/.bin/ngserver',
\ 'args': [
\ '--stdio',
\ '--tsProbeLocations',
\ g:lib_path . 'node_modules/typescript/lib/',
\ '--ngProbeLocations',
\ g:lib_path . 'node_modules/@angular/language-server/bin/',
\ ],
\ 'initializationOptions': {'diagnostics': 'true'},
\ 'runIfSearch' : [
\ 'angular.json',
\ ],
\},
\{
\ 'name': 'htmlls',
\ 'filetype': ['html', 'php'],
\ 'path': g:lib_path . 'node_modules/.bin/html-languageserver',
\ 'args': ['--stdio'],
\ 'initializationOptions': {'embeddedLanguages': {'css': v:true, 'javascript': v:true}},
\ 'syntaxAssociatedLSP': ['htmlTag', 'htmlError'],
\ 'runUnlessSearch': [
\ 'angular.json',
\ ],
\},
\])
|
i know, so i am just discussing, not against it, but just the detection on syntax val seems manually seems was not reliable, or IF it would be auto then would be great. as for 'ansible', there was no fixed file like |
3b08cf2
to
8110f09
Compare
.gitignore
Outdated
@@ -6,3 +6,4 @@ test/X*.cpp | |||
# test/Xtest.c | |||
# test/Xtest.cpp | |||
test/results.txt | |||
test/dummy-lsp/node_modules/ |
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.
if so, no such dir or not req now.
17b70f1
to
555c240
Compare
Manually configuring the syntax may seem non-optimal. But this can have certain advantages, notably allowing the user to adapt their configuration as they wish without having to worry about whether the LPS plugin supports the scenario or not. On the other hand, this can in fact sometimes be tricky. I actually got trapped with the new unit tests because on my machine a C language block is recognized with the syntax name As there is no standard regarding the name of syntax identifiers and each syntax description has its own terminology, I do not see how to automate the process of identifying syntax names depending on the context. Any idea suggested? |
i like your 'try', but so far no good answer, wish there some more convenience or reliable way. |
Would this fix #521 ? |
Improve multi-language server support by enabling the selection of the appropriate LSP based on syntax stack elements using a new configuration option for the LSP:
syntaxAssociatedLSP
, which accepts a list of syntax names obtainable through:This enhancement allows by example, the simultaneous use of a PHP and HTML LSP in a PHP file when an LSP function is triggered. If the syntax stack contains an
htmlTag
pattern, the HTML LSP is called; otherwise, the PHP LSP is utilized. Furthermore, this improvement supports cases like embedding GraphQL blocks in TypeScript files.Example usages include situations like the following configurations:
While acknowledging that this approach is relatively straightforward, I believe it remains effective for conventional use cases as long as the language server is designed to operate within the context of the current file.