dev-cycles / contextive

Get on the same page.
https://contextive.tech
MIT License
244 stars 6 forks source link

fix(language-server): support LSP clients that only support `workspace/configuration` #58

Closed chrissimon-au closed 10 months ago

chrissimon-au commented 10 months ago

To compensate for https://github.com/OmniSharp/csharp-language-server-protocol/issues/1101, switch to using scoped configuration method which doesn't (incorrectly) rely on the didChangeConfiguration capability from the client.

Added a test to simulate the scenario we see with neovim where it offers the workspace.configuration capability but not the workspace.didChangeConfiguration capability. (This test failed before the change, and went green after the change).

Refactored the test helper ConfigurationSection to simplify that scenario, and to more generally rely less on didChangeConfiguration capability for tests that only use static setting values.

Due to the aforementioned OmniSharp bug, Contextive was not able to obtain configuration settings from neovim client. A workaround was explored by @erikjuhani on #55, but now that we know the root cause is a library bug, this is the minimally disruptive solution.

By using the GetScopedConfiguration, we bypass the didChangeConfiguration capability check. See https://github.com/OmniSharp/csharp-language-server-protocol/issues/1101 for details. We should revert this change (but not the tests) after that issue is resolved.

Also, now that this works, the neovim README.md section has been updated to illustrate how to nominate a custom definitions file location.

No.

See also #54 for other in-filght considerations around the configuration story.

erikjuhani commented 10 months ago

Thank you, @chrissimon-au!! I tried the newest main, and I can confirm that it works as expected now! 🎉 Can you create a new release so that it can be installed through Mason?

chrissimon-au commented 10 months ago

Yes - release coming, just investigating some intermittent test failures that have recently cropped up. I suspect to do with unrelated changes (I have a temporary 'survey prompt' popup in there at the moment that might be confusing some of the tests). But I'm reluctant to cut a release while the tests aren't reliably passing - hopefully get it sorted out today then will cut that release!

erikjuhani commented 10 months ago

Yes - release coming, just investigating some intermittent test failures that have recently cropped up. I suspect to do with unrelated changes (I have a temporary 'survey prompt' popup in there at the moment that might be confusing some of the tests). But I'm reluctant to cut a release while the tests aren't reliably passing - hopefully get it sorted out today then will cut that release!

Thanks for the update! And no worries about the release; there's no rush from my end at least!