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

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

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

chrissimon-au
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

To compensate for OmniSharp/csharp-language-server-protocol#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.

  • What is the current behavior? (You can also link to an open issue here)

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.

  • What is the new behavior (if this is a feature change)?

By using the GetScopedConfiguration, we bypass the didChangeConfiguration capability check. See OmniSharp/csharp-language-server-protocol#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.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

  • Other information:

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

…e/configuration`.

See OmniSharp/csharp-language-server-protocol#1101 for the upstream bug that necessitates this.  We can revert to unscoped configuration when that is resolved.
@erikjuhani
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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!

chrissimon-au pushed a commit that referenced this pull request Nov 15, 2023
## [1.10.3](v1.10.2...v1.10.3) (2023-11-15)

### Bug Fixes

* **language-server:** support LSP clients that only support `workspace/configuration` ([#58](#58)) ([7e06396](7e06396))
* **language-server:** survey prompt more resilient to accidentally missing the first prompt ([7acd140](7acd140))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants