Clojurians Log v2

wait to see what you can do with https://clojure-lsp.io/features/#execute-command

https://clojure-lsp.io/features/#documentation-and-clojuredocs-integration

@ericdallo I submitted a draft pull request. It's only for you to review right now, I might have to do some more work. One issue is that this new extract-function requires changes to the client, so I created a new extract-function-2. That's not ideal, but I'm looking for your feedback. I've made the required changes to calva and they are working OK for me so far. https://github.com/clojure-lsp/clojure-lsp/pull/2169

Ergonomically, it feels nice to aim my selection at the general area I want extracted. But... sometimes it can be surprising. And a selection that spans multiple defns would have to be disallowed I think.

yeah, I believe most users using this will know that is expected to select a valid range, so I'd go with that

I see that the Windows build failed on github with a AccessDeniedException, but it looks like at least some (that I checked) PRs fail for Windows with the same error

This is the "Extract Function 2" running

hey @john.t.richardson.dev, that looks awesome! I'm a little bit busy this week but I will take a look at your PR and think on an approach for us to deliver that, meanwhile, maybe you can elaborate why we need changes on client just to make sure we should go with this approach and think how to offer as the default for users in a non breaking change way

Sure, no rush. I should take some time to look at the emacs situation anyway. In the meantime... The issue in Calva is that it changes the default function name by changing the arguments array entry in the LSP communication. Here's what Calva is sending back when the original extract-function is invoked:

{
    "command": "extract-function",
    "arguments": [
        "file:///home/john/Projects/clojure-lsp/lib/src/clojure_lsp/refactor/mytest3.clj",
        15,
        2,
        "new-fn"    <--- arguments[3] is the new fn name, as changed by Calva in lsp-commands.ts
    ]
}

(defn ^:private extract-function-action [uri line character]
  {:title   "Extract function"
   :kind    :refactor-extract
   :command {:title     "Extract function"
             :command   "extract-function"
             :arguments [uri line character "new-function"]}})

{
    "command": "extract-function-2",
    "arguments": [
        "file:///home/john/Projects/clojure-lsp/lib/src/clojure_lsp/refactor/mytest3.clj",
        15,
        0,
        16,
        19,
        "new-fn"   <--- now at argument[5]
    ]
}

-      args[3] = command.defaultName;
+      args[command.namePosition ?? namePositionDefault] = command.defaultName;

Doh! I just realized I might be able to put the additional selection location arguments at the end of the list. This might work around the problem with the Calva client modifying the arguments array at a hardcoded position. I should try that!

Ah got it, the LSP arguments spec kinda sucks in that aspect, not being able to increase and improve a command Question: can we avoid a breaking change keeping the existing arguments order and just add new ones as the later? so we can in server control if that is a new extract-function or not keeping old behavior

ah yeah, that's what I meant :)

Right, good idea. I'll look into it

thanks! looking good

@ericdallo I got this working and yeah, no Calva changes are required now. Should I add a (perhaps undocumented?) feature flag to revert back to the old extract-function code, or should I just replace it?

That's awesome! I'm pretty ok to replace it if we add tests testitng old and new behavior

OK. I've repurposed the existing extract-function tests and added a bevy of new ones. I'll send a new PR in a day or two. I just want to live with the changes for a few days myself.

Cool, Sounds like a good idea!