Fork me on GitHub

i've scanned the doc site, but i'm not finding it. how do i stop all those refactoring buttons from showing in my editor?


I think it’s (setq lsp-ui-sideline-show-code-actions nil)


@U0509NKGK fwiw I don't use lsp-ui at all


I find it too much

😅 2

fantastic guide, thanks!


if you don't use the ui at all, are you manually invoking specific lsp commands then, @U04V15CAJ?


I mostly use navigation, hardly any other features, but only the navigation is worth it for me ;)


you mean the jump to def / find refs stuff? totally with you if so


also find refs, indeed


Actions are useful too (as shortcuts, not via lsp-ui)


I have sideline errors turned on, as otherwise they show up in the echo area, and I don’t see the signatures in there.


But yeah, lsp UI is a bit much


I just use it for errors


Me too, just the errors, code actions are already available on modeline with the bulb 💡 icon


-uninstalls lsp-ui- will see how it goes


BTW, we changed that variable on lsp-ui to be off by default, maybe you have a outdated lsp-ui?


defo an older install, i think!


@ericdallo Thanks for adding the cursor handling to move-coll-entry. That change broke the tests, but I’ve got them working again. I haven’t looked at writing tests that the cursor ends up in the right place, but I plan to


Np, yeah, I'm about to fix the tests, I just had to leave the PC, I intend to fix today + add code actions tests


I fixed some things that were not working even on master


Ok cool. I’ll focus on adding tests and seeing if I can cover more cases.


You left a question about whether we really need :track-position? globally. It could be removed if we’re ok with leaving the whitespace messy in certain cases. With it:

{:a 1 ;; one
     :b 2}

; move :a down -->

    {:b 2
     :a 1 ;; one


Without it:

{:a 1 ;; one
     :b 2}

; move :a down -->

    {:b 2
     :a 1 ;; one


Notice the position of the closing bracket in the later case


Lots of users won’t care about this, because their editor manages whitespace for them. But I’ve never been able to get those tools to work the way I want so I typically turn them off


I like doing the “right thing” but if it introduces performance problems or anything like that, it’s probably OK to abandon


exactly, I'm ok keeping that if no performance issue, but since we would need to change a code where it parses the loc and could affect a lot of places, not sure


that s why I asked it on #rewrite-clj


Ah, I didn’t see that question. Thanks for asking it. I’ll experiment with using the node metadata. That may be sufficient


yes it that works, perfect, otherwise I'm ok with leaving that bracket not indented for now, we can try to improve on a later interaction


Just tried it out. The node metadata works! Or at least my tests still pass. 🙂 I’ll remove :track-position? from the codebase. Anything else I need to change?


nice, I think only removing from the parser.clj is enough


BTW the cursor change after the move up/down command is not working yet, I'm debugging it


what makes or should make this feature works is the show-document-after-edit


I think it's related with the fact we are editing the node manually not using z/subedit-> which should update the meta location properly


I noticed that code, but wasn’t sure what it did. Actually, thinking about it, that may break without :track-position? . I don’t exactly know what :track-position? does, but my guess is that when you edit a node it recursively recalculates the position of all the children. So without :track-position?, the new-zloc will still have its old position, meaning that show-document-after-edit wouldn’t change positions.


yes, the old position is being used, but not sure it's related with track-position since we use the node position on a lot of places, probably all refactors


my guess is related with we manually dropping/inserting nodes without using z/edit or z/subedit


That’s possible. It may also be that meta isn’t being updated. That is, I wonder if the problem is actually that you’re calling (meta (z/node new-loc)) instead of using z/position or z/position-span.


hum, we use (meta (z/node new-loc)) all over clojure-lsp and never found a issue, I think there was no z/position at that time as well 😅


it worth checking both anyway


OK. I meant to ask whether z/edit or some variation was more correct. I tried that early on but couldn’t get it working


there are a lot of places using on clojure-lsp, maybe it could help, check transform.clj


but basically with z/subedit-> you can navigate through the node and change anything, and after that call it will return the node editted


at least this is as I always used it and it worked


OK, I’ll plan to try that out. I’m working on something else at the moment

👍 1

If you track down what’s going on, let me know. Otherwise I’ll plan to look at it a bit later

👍 1

@U07M2C8TT I managed to fix the focus/cursor location but with a workaround, as I tried using z/subedit but it didn't fix the cursor position


I'm ok with that workaround, but if you want to find a better fix, I'd appreciate


Hmm, I haven’t been able to get z/subedit to work at all. Is the workaround pushed? If so, I’ll take a look and see if I can find a better fix


Yes, lastest commit contains it


Out of curiosity, I tried :track-position? again. With that, I can use z/position-span to get the correct cursor location. I still don’t exactly understand why it’s necessary in move_coll_entry.clj but not in transform.clj. Perhaps because of the way move-coll-entry tries to re-navigate to an existing node instead of to a brand new node. Anyway, that’s one route. I won’t commit these changes since I still think it’s best to avoid :track-position? , but it’s interesting to know


> Perhaps because of the way move-coll-entry tries to re-navigate to an existing node instead of to a brand new node. Yeah, I guess that is the reason Good to have that, we probably need to do some benchmark of how this affect clojure-lsp performance, but sounds like something we could use in the future


BTW what editor do you use @U07M2C8TT?


I use Emacs. Why do you ask?


I made some changes on lsp-mode to allow that refactoring:

(defun lsp-clojure-move-coll-entry-up ()
  "Apply move coll entry up refactoring at point."
  (lsp-clojure--refactoring-call "move-coll-entry-up"))

(defun lsp-clojure-move-coll-entry-down ()
  "Apply move coll entry down refactoring at point."
  (lsp-clojure--refactoring-call "move-coll-entry-down"))
and added bindings on my emacs config:
(define-key lsp-mode-map (kbd "C-M-<up>") #'lsp-clojure-move-coll-entry-up)
(define-key lsp-mode-map (kbd "C-M-<down>") #'lsp-clojure-move-coll-entry-down)
In case you are interested :) I can commit on lsp-mode after merging this clojure-lsp feature


Oh excellent! It didn’t occur to me that lsp-mode needs parallel changes.


only if someone is intended to use the refactoring manually instead of the code actions


Gotcha. I like the code actions menu, but if you want to invoke an action repeatedly, as in this case, the manual keybindings are useful


I made a for where I’m at. It has a few fixes but mostly introduces several commented-out failing tests. I have a plan for fixing some of those, though probably not all of them.


If you want, you can pull that into your branch. If you do, let me know and I’ll make another PR for further changes


I just checked that, looks good to me to merge to the branch, I'm ok not covering all cases, having commented tests for those already helps a lot if we want to fix that in the future


I just merged your branch, I'll work on the docs + changelog


Ah, ok, thanks!


feel free to tell me when you think the branch is good enough to be merged to master


@ericdallo I just created a with the changes I wanted to make. I have one open question which I asked in the PR. After that, it’ll be ready to be merged to master


Great! Thanks for your help putting it together. I’m excited to use it. And thanks in general for maintaining this project. From the moment I started using it, my experience with my code and editor felt much richer. I sometimes have time to work on this kind of project so if you have other features you’d like to see developed, ping me and I’ll see if I can help.


Thank you very much! Glad to hear it! Feel free to help on the other issues or new ones 😛


Your fixes worked perfectly, way better than before indeed, awesome work!


Merged the branch to master, thanks again!


:thumbsup: Good to hear. The underlying algorithm is much more sound now.


The one case I know of that it doesn’t handle very well is moving entries with both commas and comments. This:

{:a 1, ;; one comment
 :b 2}
Becomes this:
{;; one comment
 :b 2, :a 1}
Which is pretty unexpected. This would be OK:
{:b 2
 :a 1, ;; one comment
And this would be ideal:
{:b 2,
 :a 1 ;; one comment
Either one would be tricky to implement, at least without breaking other things.


As a first iteration I'm really OK with the current behavior, feel free to improve if you want


Me too. I’m gonna leave it as is for now. If it bothers me, I’ll see if I can fix it. But commas are rare enough that I doubt I’ll ever notice

👍 1

Also, FWIW, the new comment allocation algorithm has the underpinnings to support @U0BUV7XSA’s suggestion of moving entries in maps and bindings, but elements in other collections. But to do that I’d still need to answer the question about how to detect whether a vector is a binding or a regular vector. Perhaps I’ll look into that more sometime.


cool, If I'd guess, I'd say cursive has some hardcode full qualified symbols that are considered entries, like clojure.core/let , we know that it doesn't work for all cases, specially user macros, but it should work for at least 90%


> > cursive has some hardcode full qualified symbols That would make sense. Two questions… 1. Is it hard to get access to a fully-qualified symbol from the clojure-lsp refactoring tools? 2. Does the resolve-macro-as! machinery attach metadata anywhere that would let you detect that the user intends an arbitrary form to be treated as a clojure.core/let?


1. no, we have kondo analysis, it should be pretty simple 2. no, the resolve-macro-as in the end just add a entry to .clj-kondo/config.edn in a :lint-as


So clj-kondo doesn’t convey how it resolved a macro?


Also, looking at resolve-macro-as!, I see that I hadn’t thought about clojure.core/for . It establishes bindings itself, but has :let inside of it, which establishes more bindings! And there are actually many more binding-establishing forms in clojure.core than I original thought of. I had come up with let, binding, with-redefs and with-local-vars , but there’s also doseq, dotimes, if-let , if-some, loop , when-first, when-let, when-some, with-open and perhaps more. (Searched for “binding” A few of those only have one entry, so there’s not really a need to support movement, but still, I think you’d want to be in the “move two” mode instead of “move one” when in those forms.


yes, a lot of cases indeed 😅 not sure there is another solution