Skip to content
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

Only call workspace/configuration when available #2843

Merged

Conversation

nospam2998
Copy link

@nospam2998 nospam2998 commented Sep 9, 2024

Happy first birthday, issue #2318!

During its first year of existance, issue #2318 has failed to get any attention at all from the project maintainers. Even though it addresses a fundamental bug. A couple of people have commented that the patch has helped them, from which we can derive that probably many more are using the patched version without bothering to comment.

Not that I believe the existence of a pull request will change anything, but lets give this a try.

As previously communicated, I do not read simplified chinese. So someone else will need to update the remaining test cases or translate and rework the explanatory comments into English. Disabling or removing broken tests would be an option as well, of course. Yet, then again, with other tests already failing on master one might as well also consider merging this commit as is.

Commit message follows: (written a year ago when first submitted, but the basic facts hasn't changed.)

Not all clients implement the client capability: configuration, which was added in version 3.6.0 of the Language Server Protocol. The LSP Specification also states:

A missing property should be interpreted as an absence of the capability.

Above claims are possible to verify by reading the mentioned spec. at: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration

Hence this change modifies behaviour to only call the method on clients explicitly announcing to support it.

Most affected test-cases are updated to work with this commit, however one test gets disabled. That disabled test suite is in serious need of added documentation explaining its design. The few comments which are there seem highly unsufficient, and since they are written in simplified chinese they practically are of no use to most potential contributors.

This commit makes the lua-language-server work with vim-ale.

test.lua Outdated Show resolved Hide resolved
@tomlau10
Copy link
Contributor

tomlau10 commented Sep 9, 2024

Not that I believe the existence of a pull request will change anything, but lets give this a try.

I think this project is still actively being developed, you can see that recent pull requests are still getting merged. Only that the author is focusing on a heavy revamp of the project, and unless the issue is a very critical one which affects almost everyone, he may not spend time to debug and fix it. 😄

Yet, then again, with other tests already failing on master one might as well also consider merging this commit as is.

AFAIK all tests are working on master currently, you can see that in repo's Actions page. But as you have said you committed it a year ago, maybe at that time the tests are failing 🤔

@nospam2998 nospam2998 force-pushed the fix/honour_configuration_capability-master branch from e5fbaa4 to 27d1676 Compare September 9, 2024 16:38
Not all clients implement the client capability: `configuration`, which
was added in version 3.6.0 of the Language Server Protocol. The LSP
Specification also states:

> A missing property should be interpreted as an absence of the capability.

Above claims are possible to verify by reading the mentioned spec. at:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration

Hence this change modifies behaviour to only call the method on clients
explicitly announcing to support it.

This commit makes the lua-language-server work with vim-ale.

Thanks to Tom Lau for assistance in getting the test-setup updated.
@nospam2998 nospam2998 force-pushed the fix/honour_configuration_capability-master branch from 27d1676 to c900372 Compare September 9, 2024 16:54
@nospam2998
Copy link
Author

... unless the issue is a very critical one which affects almost everyone, he may not spend time to debug and fix it. 😄

The issue is quite critical. It makes LuaLS completely useless with many (standard compliant) clients. Isn't VSCode one of the few it actually does work with?

Issue #2318 is one year old today, with debug and fix available for about a week less than that. Plus, there were/are at least two duplicates. I can definitely understand how one avoids un-actionable issues, but all I asked for was help with the test cases. Test cases which are opaque to anyone not reading simplified chinese.

AFAIK all tests are working on master currently, you can see that in repo's Actions page.

Working on some random docker container and passing in real environments are not the same thing. There is an issue for the ones I did see fail (maybe it actually got fixed recently, didn't see any real breakage now).

@tomlau10
Copy link
Contributor

tomlau10 commented Sep 10, 2024

It makes LuaLS completely useless with many (standard compliant) clients. Isn't VSCode one of the few it actually does work with?

I have no idea, I use VSCode 😂. The other editor that I have heard the most in the project's issue / discussion page is NeoVim (though I know nothing about it), and seems no one there is complaining that LuaLS is broken for them. I know Vim (again not familiar, at least I know how to quit it 🤣), but this is the first time that I heard of vim-ale.

Even though VSCode might be one of (or even the only one) that luals will work, I think the user proportion of VScode is the largest among all the editors right now 🤔 From wiki page (https://en.wikipedia.org/wiki/Visual_Studio_Code), in 2023 70+% respondents are using VScode (including the luals author's team AFAIK). So if luals works in vscode, certainly this issue is not affecting almost everyone. (affecting many editors != affecting many users)

but all I asked for was help with the test cases. Test cases which are opaque to anyone not reading simplified chinese.

Authors speak chinese (so as I), and seems that they read english using translation software. They may skip issues which are very long and written in pure english due to hard to understand. Maybe next time you can try to translate (google / chatgpt) to chinese first (along with your original text in english ofc), might as well use @mention too (I don't see any in your #2318) to get their attention 😄


Anyway glad that now the PR pasts all the tests 🎉 (yeah since the tests passed, now it doesn't matter what they do 🙂)
I think it's good to go and let's wait for maintainer to merge it 👍

@CppCXY
Copy link
Collaborator

CppCXY commented Sep 10, 2024

When implementing the IntelliJ version of luals, I initially planned to resolve this issue, but lsp4ij proactively implemented a workspace/configuration that returns null values, which subsequently demotivated me from pursuing these tasks further.

@CppCXY CppCXY added the enhancement New feature or request label Sep 10, 2024
@sumneko sumneko merged commit e87d7d6 into LuaLS:master Sep 10, 2024
11 checks passed
@nospam2998
Copy link
Author

Thanks @tomlau10, @CppCXY, @sumneko for proving me wrong in my expectation of a PR being treated no differently than the year old issue!

@sumneko
Copy link
Collaborator

sumneko commented Sep 10, 2024

All issues can be attributed to the fact that I don't have time right now. I haven't read most of the issues, but if you've opened a pull request and there are no obvious problems, I will accept it.

@nospam2998 nospam2998 deleted the fix/honour_configuration_capability-master branch September 10, 2024 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants