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

Remove the display diagnostics setting in favor of managing diagnostics entirely via clojure-lsp config #1067

Closed
bpringe opened this issue Mar 15, 2021 · 5 comments
Labels

Comments

@bpringe
Copy link
Member

bpringe commented Mar 15, 2021

https://clojure-lsp.github.io/clojure-lsp/settings/#diagnostics-linter

Currently we just intercept diagnostics in the lsp middleware, but perhaps a better method is to modify the lsp config. I'm not sure though if this is wise. To disable diagnostics for all projects, we'd need to modify the users ~/.lsp/config.edn, and then modify it again if we want to enable them.

Would modifying the user's global lsp config pose any issues? I guess another thing to consider is would the change be immediately noticeable. With the current implementation using middleware, the change is immediate. We could always keep the current method for the immediate effect, plus modify the config if that actually prevents some work being done on the server.

@ericdallo Does turning linting off in the lsp config cause less work by the server? I guess it at least makes it not send the diagnostics notification, but does it calculate them for other things anyway?

@ericdallo
Copy link
Contributor

You don't need to change user config, you can pass those same configs via initializationOptions :)

@ericdallo
Copy link
Contributor

the server will still calculate since we use for other features, but we will not publish them, avoiding unnecessary json to client, eventually improving performance

@bpringe
Copy link
Member Author

bpringe commented Mar 15, 2021

Got it, thanks! Since this wouldn't take effect until next startup though, I think it makes sense to do this along with the existing middleware usage we have. Btw, am I understanding correctly that the initializationOption setting is :linters (plural)? If so, it may need to be corrected here: https://clojure-lsp.github.io/clojure-lsp/settings/#all-settings

@ericdallo
Copy link
Contributor

Yes, I'll fix the docs, thanks!

@bpringe bpringe added this to To do in Brandon's Board via automation Mar 17, 2021
@bpringe bpringe moved this from To do to In progress in Brandon's Board Mar 31, 2021
@bpringe bpringe changed the title Change Display Diagnostics setting to use the clojure-lsp and clj-kondo lint configs Remove the display diagnostics setting in favor of the user managing diagnostics entirely via clojure-lsp config Mar 31, 2021
@bpringe
Copy link
Member Author

bpringe commented Mar 31, 2021

Removing this setting after discussing in Slack.

@bpringe bpringe changed the title Remove the display diagnostics setting in favor of the user managing diagnostics entirely via clojure-lsp config Remove the display diagnostics setting in favor of the managing diagnostics entirely via clojure-lsp config Mar 31, 2021
@bpringe bpringe changed the title Remove the display diagnostics setting in favor of the managing diagnostics entirely via clojure-lsp config Remove the display diagnostics setting in favor of managing diagnostics entirely via clojure-lsp config Mar 31, 2021
Brandon's Board automation moved this from In progress to Done Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

2 participants