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

Provide option to completely disable LSP #4425

Merged
merged 3 commits into from
Feb 2, 2023

Conversation

dead10ck
Copy link
Member

@dead10ck dead10ck commented Oct 23, 2022

I had had this code sitting around in a branch I'm working on already before #4391 became necessary in order to turn off LSP for the integration tests. It slows them down and is totally unnecessary until we figure out a good way to do integration tests with real language servers.

I'm making this PR as a proposed different way to approach this, where this is generalized into a user config option exposed to the user. I figure that while my motivation for this was reducing noise in the integration tests, this could conceivably be desirable by some users without having to e.g. compile out LSP integration with a feature flag.

Another benefit is that when we do get to integration tests around LSP, we won't need to manually specify a language config; we'll just need to set this one boolean config setting in the test invocation, which is a lot less tedious, and allows always testing the same default configs that ship to users.

With that said, I don't actually have any strong feelings about keeping this, I just thought it was worth proposing and seeing what everyone thinks since I had already done it. I'm perfectly content leaving it the way it is now.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good - I would prefer an option like this to the change in #4391 (removing LSP config by deleting the toml keys). I think this just needs a new line in the book for the new option:

### `[editor.lsp]` Section

@dead10ck
Copy link
Member Author

Done

@dead10ck dead10ck mentioned this pull request Oct 24, 2022
@dead10ck dead10ck force-pushed the disable-lsp branch 2 times, most recently from 201eeb3 to 803a8cc Compare October 24, 2022 05:02
@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 25, 2022
@dead10ck
Copy link
Member Author

@archseer any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants