This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2022-01-07
Channels
- # adventofcode (3)
- # announcements (6)
- # babashka (20)
- # beginners (53)
- # calva (11)
- # clj-kondo (11)
- # clojure (50)
- # clojure-argentina (4)
- # clojure-dev (1)
- # clojure-europe (14)
- # clojure-houston (1)
- # clojure-italy (2)
- # clojure-nl (4)
- # clojure-norway (3)
- # clojure-seattle (3)
- # clojure-uk (13)
- # clojurescript (2)
- # cloverage (1)
- # code-reviews (4)
- # conjure (2)
- # cursive (5)
- # datalevin (4)
- # datascript (33)
- # datomic (16)
- # events (1)
- # graphql (10)
- # gratitude (1)
- # honeysql (6)
- # introduce-yourself (2)
- # jobs (1)
- # lsp (88)
- # malli (8)
- # off-topic (3)
- # other-languages (4)
- # polylith (3)
- # re-frame (16)
- # reagent (17)
- # reitit (3)
- # releases (2)
- # remote-jobs (1)
- # rewrite-clj (3)
- # shadow-cljs (3)
- # slack-help (2)
- # sql (36)
- # testing (31)
- # tools-deps (41)
- # xtdb (23)
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?
@U0509NKGK fwiw I don't use lsp-ui
at all
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
I have sideline errors turned on, as otherwise they show up in the echo area, and I don’t see the signatures in there.
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
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
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?
BTW the cursor change after the move up/down command is not working yet, I'm debugging it
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 😅
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
If you track down what’s going on, let me know. Otherwise I’ll plan to look at it a bit later
@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
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
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."
(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 featureOh 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 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.
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
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 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
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 😛
: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
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” 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.