Fork me on GitHub
#lsp
<
2022-01-07
>
robert-stuttaford11:01:28

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?

djm11:01:12

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

borkdude11:01:58

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

borkdude12:01:00

I find it too much

😅 2
robert-stuttaford12:01:47

fantastic guide, thanks!

robert-stuttaford12:01:53

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

borkdude12:01:47

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

robert-stuttaford12:01:32

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

borkdude12:01:39

also find refs, indeed

mpenet12:01:36

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

djm12:01:44

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

mpenet12:01:52

But yeah, lsp UI is a bit much

mpenet12:01:10

I just use it for errors

ericdallo12:01:32

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

robert-stuttaford17:01:49

-uninstalls lsp-ui- will see how it goes

ericdallo17:01:36

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

robert-stuttaford13:01:25

defo an older install, i think!

jacob.maine19:01:30

@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

ericdallo19:01:34

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

ericdallo19:01:52

I fixed some things that were not working even on master

jacob.maine20:01:51

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

jacob.maine20:01:38

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
     }

jacob.maine20:01:53

Without it:

{:a 1 ;; one
     :b 2}

; move :a down -->

    {:b 2
     :a 1 ;; one
}

jacob.maine20:01:09

Notice the position of the closing bracket in the later case

jacob.maine20:01:59

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

jacob.maine20:01:26

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

ericdallo21:01:45

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

ericdallo21:01:54

that s why I asked it on #rewrite-clj

jacob.maine22:01:36

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

ericdallo22:01:46

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

jacob.maine22:01:19

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?

ericdallo22:01:19

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

ericdallo22:01:49

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

ericdallo22:01:11

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

ericdallo22:01:55

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

jacob.maine22:01:56

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.

ericdallo22:01:02

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

ericdallo22:01:23

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

jacob.maine22:01:18

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.

ericdallo22:01:20

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 😅

ericdallo22:01:36

it worth checking both anyway

jacob.maine22:01:49

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

ericdallo22:01:20

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

ericdallo22:01:11

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

ericdallo22:01:26

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

jacob.maine22:01:36

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

👍 1
jacob.maine22:01:43

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

👍 1
ericdallo00:01:45

@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

ericdallo00:01:12

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

jacob.maine00:01:01

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

ericdallo00:01:45

Yes, lastest commit contains it

jacob.maine00:01:29

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

ericdallo00:01:25

> 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

ericdallo01:01:14

BTW what editor do you use @U07M2C8TT?

jacob.maine20:01:10

I use Emacs. Why do you ask?

ericdallo20:01:31

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."
  (interactive)
  (lsp-clojure--refactoring-call "move-coll-entry-up"))

(defun lsp-clojure-move-coll-entry-down ()
  "Apply move coll entry down refactoring at point."
  (interactive)
  (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

jacob.maine21:01:42

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

ericdallo21:01:26

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

jacob.maine21:01:42

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

jacob.maine21:01:39

I made a https://github.com/clojure-lsp/clojure-lsp/pull/686 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.

jacob.maine21:01:40

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

ericdallo21:01:12

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

ericdallo21:01:18

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

jacob.maine21:01:37

Ah, ok, thanks!

ericdallo21:01:08

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

jacob.maine06:01:40

@ericdallo I just created a https://github.com/clojure-lsp/clojure-lsp/pull/688 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

jacob.maine18:01:25

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.

ericdallo21:01:55

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

ericdallo00:01:32

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

ericdallo00:01:44

Merged the branch to master, thanks again!

jacob.maine19:01:40

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

jacob.maine19:01:43

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.

ericdallo19:01:40

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

jacob.maine19:01:46

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
jacob.maine19:01:53

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.

ericdallo19:01:23

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%

jacob.maine20:01:10

> > 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?

ericdallo20:01:38

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

jacob.maine20:01:49

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

jacob.maine20:01:18

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” https://clojure.github.io/clojure/clojure.core-api.html.) 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.

ericdallo20:01:44

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