Add a configuration option to disable diagnostics

[plexus]

What has Changed?

A boolean config flag displayDiagnostics was introduced, it defaults to true. When it is set to false, then any diagnostics (i.e. linter warnings) coming from clojure-lsp are ignored.

See #980 for the motivation behind this.

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• Tested locally by building the vsix, unzipping it, and copying extension to ~/.vscode/extensions
• Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: There is a CircleCI bug that makes the Artifacts hard to find. Please see this issue for workarounds.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • If I am fixing the issue, I have used GitHub's fixes/closes syntax
  • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• Created the issue I am fixing/addressing, if it was not present.
• Added to or updated docs in this branch, if appropriate

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

[PEZ]

Hello, many thanks for helping out with this!

I see three somewhat separate questions here:

1. Why does configuring this through clj-kondo configs not work?
2. Should Calva have this setting?
3. If so, how should the setting be implemented?

The place for discussing 1 is probably #980, but I'll ping in @bpringe and @ericdallo here anyway for their context. I don't right now recall the status of things in regards to how the clj-kondo inside clojure-lsp is configured, but it would not surprise me if clojure-lsp/clojure-lsp#261 can offer some hope for fixing #980.

As for the setting's be or not be:

• I think I am for it as something I want to the users to have. It's a bit opaque to the user how clj-kondo fits into the picture, so for many it will be a bit far fetched to figure out that the linting can be switched off with a clj-kondo config file. Let's see what @bpringe's take is.
• Something that might make it a bit difficult to implement is if the user wants different settings for different subdirectories/subrepos. Maybe the settings could be considered a master switch here and for more granular control the user would need to have linting enabled and then use clj-config files to control it in per directory tree.

As for implementation of the setting, some things we should consider, imo:

• Most Calva settings take effect when they are changed. This PR's way of implementing the setting adheres to this pattern.
• Can/should the setting be done through configuring the clojure-lsp server?
  • If so, can we configure it on the fly or would it take a server restart?
  • If it takes a server restart, is that instant enough?
• Should the setting be implemented by Calva editing the clj-kondo config file? This would also adhere to the instant effect, I think, because that is how clj-kondo works, right?

As far as a master switch goes, I think it makes sense to try to stop linting from even happening if someone switches it off.

I will let the answer to question 1. above guide me a bit. If it is something we can assume is about to be fixed, then maybe wait with implementing a setting until we get indications that many users want to disable and have trouble figuring out how. If the fix is a way away, then maybe pull this PR as it is as a temporary fix. (Not sure, since giving the users the setting easily gets a commitment to keep it.)

[ericdallo]

About 1:
We from clojure-lsp are working hard last and this week on clojure-lsp/clojure-lsp#261 which is a huge refactor that will make clojure-lsp use 100% clj-kondo for almost everything, therefore IMO this issue would be closed since user would need just to disable clj-kondo lint as it already has this possibility from clj-kondo side, WDYT?

[plexus]

Thanks a lot for the input @PEZ and @ericdallo. It's good to see clojure-lsp using the clj-kondo linters for unused-ns/unused-public, this was definitely a source of confusion.

IMO for something that is so invasive in terms of UX deserves a clear master switch inside the Calva settings. That's where people will look for it. Ideally this should signal to clojure-lsp that no linting is wanted/needed, so that clojure-lsp can skip invoking clj-kondo. No wasted cycles just to start it up and do nothing. No creating .clj-kondo/cache which remains empty.

If people want this toggle on a per project level they should be able to do it in .lsp/config.edn, something like {:clj-kondo false} for instance. If they want to fine tune the linters they can use .clj-kondo/config.edn (or .lsp/config.edn.

But telling people that if they are not interested in clj-kondo that they should just disable all linters doesn't seem right. Why do I need to configure a tool I don't want? It's also not clearly documented how one is supposed to do that. The clj-kondo docs don't mention it, since indeed why would they? That's not what you're there for. I think {:linters ^:replace {}} should do the trick, but you have to admit that that's not blindingly obvious. (not to mention ~/.clj-kondo/config.edn vs .clj-kondo/config.edn vs :clj-kondo in .lsp/config.edn)

So that's my proposal

• have a flag at the clojure-lsp level to not invoke clj-kondo
• allow setting that from .lsp/config.edn or from Calva settings
  • if it's OFF in Calva then it's OFF (Calva setting is the master switch)
  • if it's ON in Calva, then honor project-level config (both default to ON)

I'd be happy to make PRs accordingly. Pointers and tips are very welcome since I'm fresh to both projects.

[ericdallo]

@plexus clojure-lsp uses clj-kondo not just for linting, but to analyze the code to make most of features work, not sure would be possible to toggle that on clojure-lsp, it's a core part of clojure-lsp now :/

If you want just to disable limiting, the LSP client, in this case Calva, should have a feature to not produce diagnostics following LSP spec via textDocument.publishDiagnostics capability

[PEZ]

I agree with your proposal, @plexus. Specifically the version where this is a VS Code setting, which is where most people search for settings, I am pretty sure. So that part of this PR is good already.

If you want, and have the time, @plexus , I'd welcome this PR to implement it with the mechanism that @ericdallo points at. If you don't have the time, leave it open and I can take it from here. Just be informed that I think it is crazy to not use clj-kondo linting so I might not prioritize this for speedy implementation. 😄

[ericdallo]

For the implementation, check how to setup capabilities on this VSCode guide and you may need to change it here

[bpringe]

Ah I'm late to the discussion here! I saw this after replying on the related issue. I agree with you @plexus about being able to disable diagnostics without having to configure the tool you don't want. I am more certain now that a master switch in Calva settings is probably wise.

[bpringe]

Just want to note that if the disabling of diagnostics requires an lsp sever restart, and this isn't easily achieved without a VS Code restart, we could use the initialization method in combination with the middleware method, so that if the user disables diagnostics, the linting warnings disappear (not sure if this would be immediate though), and then after a restart the initialize request can configure no diagnostics.

[bpringe]

I'm thinking of merging this as-is, and creating an issue for figuring out later how to disable diagnostics from the initialization request. We'd want this existing change in combination with that anyway I think (see above comment). If no objections I'll do this soon.

[bpringe]

@plexus If you don't mind adding the change to the Unreleased section of the changelog and addressing the comment about the description, then I'd be ready to merge it.

Diagnostics are calculated/used in clojure-lsp for a feature besides just reporting the linter warnings, so trying to disable from initialization options I think is not wise, at least at this point. This middleware approach is good.

[plexus]

I'll update the description as mentioned and add a changelog entry, as requested. What's up with the back merge? Can I rebase this instead?

[bpringe]

I just wanted to bring in the latest changes from the dev branch and resolve a conflict or two. To be honest, I really just stick to merging and don't rebase, since it's the safer option and preserves the complete history.

But, it seems from my basic understanding that it wouldn't cause an issue here since you're the only one working off this branch, so feel free to rebase. 😄 My apologies if I interrupted your flow, and thanks for the changes!

[plexus]

I vastly prefer rebasing feature branches over merging upstream back. Much less confusing, and easier to see what actually changed in a given branch.

I rebased this on current dev, added a changelog entry, and dropped the "from clojure-lsp and clj-kondo" from the config flag description.

I'm sorry I couldn't polish this up better, my bandwidth for open source is a little limited at the moment.

[bpringe]

Thanks for taking the time to make those changes. I noticed one more thing when testing it and commented on the relevant line. As noted there I'll make the change if you don't have the time, no worries.

Just a note for ourselves for later, not to be included in this PR, I think we can make the diagnostics auto clear when the setting is checked by getting the current diagnostics from the lsp client, looping through the URIs, and sending an appropriate lsp notification (like didOpen) for each to make it publish diagnostics again and trigger the middleware logic. I'll make an issue for this later.

[bpringe]

With this, I noticed when the setting is unchecked, whatever linter warnings existed before are not cleared, even when the user types in the relevant files. If we return next(uri, []) then the lint warnings will be cleared when the file is changed (and maybe other events like reopening it).

I'll make this change if you don't have time, just didn't want to modify your PR without checking with you.

[plexus]

Feel free to run with it!

[plexus]

And thanks a lot!