Clojurians Log v2

dynamic code actions would be nice!

cool ! I'll clean it up and submit it for review asap

I don’t quite recall why we have manual code action. Seems there could be a reason…

the only thing I have in mind is a step that handles renames (I think my memory is really spotty)

the reason is that was not easy to have dynamic ones IIRC

oh it basically allows the user to choose the binding name instead of the generic one colure-lsp puts in

oh that's not the only thing indeed

The suspense, @fahd.elmazouni! 😃

the commands an extension exposes should be in its "manifest" ?

I don’t recognize/recall that as a reason…

Can you git blame the relevant file and see who we can ping in here?

I am of course for this being as dynamic as we possibly can get it. Just need to gain a fuller picture…

I'll have a deeper look once I get some time, I'll report back with findings and/or a pr

for now, I made claude dig the git history and it came up with this

Root cause: multi-project support broke it.

  1. Before Jan 2023 — Single LSP client per workspace. clojure-lsp auto-registered all commands via standard LSP client/registerCapability. Worked fine.
  2. Jan 10, 2023 (bc34181b6, Julien Vincent, PR #2020) — Multi-project support landed. Multiple LSP clients in same VS Code window. Auto-registration disabled because multiple clients registering same command names = conflicts.
  3. Hidden regression — Nobody realized VS Code internally binds operations like "Organize Imports" and "Add Missing Import" to these LSP commands. With registration disabled, those silently broke. No errors, just nothing happening.
  4. Jan 30, 2023 (f80217a90, fixes #2040, #2041) — Manual static registration added as fix. All commands hardcoded with logic to pick correct LSP client per file. Comment in code explains exactly this:

  ▎ "The built-in behaviour resulted in command registration conflicts. There are several vscode operations (such as organise-imports) which are bound to execute these internal lsp commands which would no longer function if these commands are not
  correctly registered.

Interesting, even so all clojure-lsp clients we always had to manually hardcode all commands, but would be nice to have that dynamically configured, only code actions work like that

Multiproject support is way more expensive than I realized when we went for it. It is very fancy and probably nice for some people who need it, but generally it is better to just run one server and configure it to support all sub projects. But that ship have sailed, I guess… Do we specify all these actions in the manifest, @fahd.elmazouni?

In any case, I suggest hard coding to match current clojure-lsp and we create an issue for looking at what we can do about making it dynamic.

TBH if a client can dynamically get the avaialble commands, I'd say that's better than hardcode

Yes, we all agree about that, @ericdallo 😃

it seems https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#executeCommandOptions in initialize result the known commands, so that's 100% possible to do!

I guess at this point adding an option to disable multi-project support wouldn't help provide a simpler path since you'd still need to support command registration and selection for multiple LSP servers? It just seems like it added a lot of complexity and breakage (and additional subsequent complexity to fix it). Maybe it isn't worth the support involved?

I can't see why this is a big deal with multiple projects, we could prefix the command with the project name or so dynamically

@seancorfield And multi-project support is still a bit broken. An option for disabling it could probably stop it from messing with projects where it is not used. It’s a separate issue from the current one though, since as long as we do have the support, we will need to account for it in code action registration.

Yeah... sometimes "good ideas" prove to not be so good once they're implemented 🙂

isn't that the story of our lives ? can't wait to sit down and have a look at this hehe

Oops, there were some new lsp code actions that I wanted, so I submitted this PR to calva to enable them: https://github.com/BetterThanTomorrow/calva/pull/3205. Does this still make sense or will everything be dynamic soon?

until we have dynamic, we might need to open PRs like that, so np to me

Ah, that’s you, @john.t.richardson.dev, I was about to ask. 😃

I think it’s good to handle the dynamism in a separate issue. Less blockage. https://clojurians.slack.com/archives/CBE668G4R/p1777988281637349?thread_ts=1777986489.734429&cid=CBE668G4R

If anyone works on that sort of thing, could it be put in its own mini library? I’d love for that to be useful for other tools. Or maybe a renewed effort on the existing library?

Yeah, I've toyed with this a few times over the years, and never quite figured out how/where to fit it into the ecosystem, but the recent (long) thread in #off-topic made me think again about doing this. I think nREPL middleware might make the most sense but, yes, as a library where the core of it could be used standalone. I just haven't dug into nREPL enough to understand all the message/response stuff.

I have a nascent nREPL middleware that rephrases / cleans up some exceptions. Example:

user=> (* 42 :x)
Execution error (UnexpectedType) at user/eval2593 (REPL:1).
Expected Number but got Keyword

It uses modified versions of clojure.main's repl-caught and err->msg -- so you could start a REPL with :caught -- and an nREPL middleware function that installs that as the ::caught-fn.

I'm asking because I'm wondering if Calva could rewrite exception messages to provide something more beginner-friendly? For example, class <source> cannot be cast to class <expected> (<source> is in unnamed module...) could be regex-mapped to something like Found <source> but expected <expected> perhaps with some class names shortened, e.g., Found Keyword but expected Number for the case I posted above (with all the module stuff suppressed). And perhaps that could be what is shown inline as well. Users can always eval *e (might be nice if there was a default Calva command and key binding for that) to get the full details?

Sweet idea! There was a project started some years ago, mapping known error messages to more beginner friendly messages. It never took off, but it was a very interesting approach. There are no hooks in Calva for doing things like that. But there could be. The messages come in via nrepl. You can enable the nrepl log and see what they look like. We do some processing on them, so that we can print the stacktrace on demand. There could be some nrepl middleware that does things like this, btw.