Clojurians Log v2

Hey @lee, first thanks for you answer! Also, before I continue, I'm learning rewrite-clj while working on that issue, so if I say or am doing anything wrong, please correct me 😇 And also, english is not my mother language, so this can also not help when I am explaining 😄

About the position mode (node vs row/col) I understand the differences, the issue is that since we are integrating with a editor, we need to use the col/row because is what the editor provide to us. The idea to use z/find-last-by-pos was to find the node based on the row/col and then work with it. I think that I did not understand your conclusion: > I think there is misbehaviour here. But I'm also not sure rewrite-clj paredit and positions work (or maybe I should say intend to work) the way you might have expected them to What do you mean by the way you might have expected them to ? I expect that if I slurp forward on this (get {}|) :a (`|` as cursor) , I should get (get {} :a) . At least in the IDE layer, I have no idea if my issue is on clojure-lsp not positioning correctly in the right node based on the row/col, or if the issue is on rewrite-clj (paredit) that should have the same behavior. Since you think there is a misbehaviour what should be the next steps? > You might have noticed some paredit fns accept a pos , I expect to compensate for this mismatch Can you please give me some example about this? I'm not pretty sure that I fully understood it.

Also about the position, I think that they are correct in the examples that I gave. For the first one, (get {}|) :a , the cursor is on {:row 1 :col 8} , right? Or I'm missing something? I understand that it returns the node (get {}) , but looking only by the position of the user's cursor, the row and col values are right.

Hiya @arthurfucher! So, a position of (get {}|) :a is not possible. You are always positioned at a node, so one of (if we include whitespace nodes): • |(get {}) :a • (|get {}) :a • (get| {}) :a • (get |{}) :a • (get {})| :a • (get {}) |:a One thing to note is that I am not the designer or original author of the paredit API, I adopted from rewrite-cljs into rewrite-clj v1.0. The paredit fns I noticed taking a pos are kill-at-pos, kill-one-at-pos and split-at-pos. My thinking is that a pos is needed because you can only navigate a zipper to a node and not a char row/col pos.

Hmm... So, I think that the root question here should be: for this code, if the actual node is the {} , it should slurp?

I currently think it should. But it doesn't. My bigger design question is the char row/col pos vs node pos mismatch.

Like, if we did not use row/col and go to the {} node, it will not work, right?

(-> "(get {}) :a"
        z/of-string
        z/down
        z/right
        paredit/slurp-forward
        z/root-string)

That's ok, we might be talking about too many things at once. I agree that all of the following should return (get {} :a) but none of them currently do:

(-> "(get {}) :a"
    z/of-string
    z/down
    z/right
    paredit/slurp-forward
    z/root-string)
;; => "(get {}) :a"

(-> "(get {}) :a"
    (z/of-string {:track-position? true})
    z/down
    z/right
    paredit/slurp-forward
    z/root-string)
;; => "(get {}) :a"

(-> "(get {}) :a"
    (z/of-string {:track-position? true})
    (z/find-last-by-pos [1 6])
    paredit/slurp-forward
    z/root-string)
;; => "(get {}) :a"

(-> "(get {}) :a"
    (z/of-string {:track-position? true})
    (z/find-last-by-pos [1 7])
    paredit/slurp-forward
    z/root-string)
;; => "(get {}) :a"

maybe paredit slurp/barf functions should receive pos as well like kill-at-pos etc? would that help solve the problem?

Yeah, there might be some design thinking to do to make the paredit API more useable. But, if I get back to my paredit bug fix effort, fixing current issues (including the one we described above) would at least improve things.

Are you guys itching to fix your bug asap? Or is it more of a casual interest at this point?

We want to fix it 😇 this is impacting some users experience

Ok, I'll crack open that old branch of mine and see where I'm at.

nice, thanks! let me know if you want more eyes on this, I do not have context/knowledge but would be nice to learn and contribute.

Probably a good idea to take a stroll through current open paredit issues: • https://github.com/clj-commons/rewrite-clj/issues/333 (our new issue) intend to fix • https://github.com/clj-commons/rewrite-clj/issues/321 intend to fix • https://github.com/clj-commons/rewrite-clj/issues/317 intend to fix • https://github.com/clj-commons/rewrite-clj/issues/257 not sure; rewrite-clj is not a formatter and hasn't typically preserved formatting. We could argue that paredit is a higher level API and do some of this. TBD. • https://github.com/clj-commons/rewrite-clj/issues/256 intend to fix

• https://github.com/clj-commons/rewrite-clj/issues/334 will fix (similar condition as our new issue but for slurp-backward)

Right... my brain is remembering while I dig in more... So here's a behaviour to explore... When at an empty seq node we slurp into that seq node (this does not match how editors work; you have to be in the seq, .i.e., [|])

(-> "[] 1 2"
    z/of-string
    pe/slurp-forward
    z/root-string)
;; => "[1] 2"

(-> "[1] 2 3"
    z/of-string
    pe/slurp-forward
    z/root-string)
;; => "[1] 2 3"

(-> "[1] 2 3"
    z/of-string
    z/down
    pe/slurp-forward
    z/root-string)
;; => "[1 2] 3"

@lee the LSP spec allow servers to have commands that may change the code and the cursor, so having paredit funcionality in clojure-lsp makes possible to have all paredit logic of all editors in a single place following a standard, currently each editor has its own way, and we want clojure-lsp-intellij to use this clojure-lsp paredit feature as the first use case, if works good we can make other editors use as well, so would be great if we could have this working in clojure-lsp, probably fixing/improving rewrite-clj if possible

I can't see other way of fixing this in clojure-lsp without fixing rewrite-clj, the logic to manipulate parens and clojure code depends pretty much 100% from rewrite-clj today in clojure-lsp

Yes but rewrite-clj works at node granularity and editors go down to char granularity. But you've been using rewrite-clj for paredit-type work already... So you might be ok with the granularity mismatch? I remember @pez did a lot of https://calva.io/paredit/. @pez did you use rewrite-clj to support paredit operations?

And the performance is good enough?

hum, yes, I was expecting ther was some way to fix that on rewrite-clj, but yeah, we do need this char granularity indeed

Let's wait to hear what pez did for Calva and then think from there.

yeah, maybe one could extract calva logic to a lib or rewrite-clj so we can re-use on multiple editors, let's see what he thinks

I would also wonder about performance. Are zippers zippy enough for this usage?

not sure performance is a problem right now, it's not something triggered automatically like editor was changed while typing multiple times, it's a command spawned by user that usually waits for it's response, so it should be fast but not ultra fast hehe

Sorry @pez, I didn't do a great job asking for your input, expertise and experience politely nor concisely. Sorry about that! lisphug? Some folks are looking to implement paredit operations using rewrite-clj.zip along with rewrite-clj.paredit . While helping to explore this option I noticed some mismatches between an editor's need to manipulate content at the char resolution and rewrite-clj operating at the node resolution. We noticed all the cool paredit work you did in Calva and wondered what you used to implement this support. Did you use/consider rewrite-clj?

I just missed the question, @lee. It looks perfectly clear to me. 😃 I didn’t know about rewrite-clj or zippers when I took part in creating Calva Paredit. I think I may have implemented it on top of those if I did it today, but I say that without having investigated it deeply at all. Calva builds Paredit on top of its structural editor, which may or may not be suitable for rewrite-clj. A very important thing with the structural editor is that code-at-rest can have structural errors and we need to not bother the users while on their way to structurally intact code. It is important to Calva that the structural editor and Paredit agree fully on the structure of the code, so that’s why they use the same infrastructure. And this is written in TypeScript and is stateful af.

Interesting, sounds like a single place having that logic in clojure would be nice

It would!

Also to note is that the structural editor is used also for things not on-demand. So performance is super important. @phill has been helping lately with attention on performance and asynchronicity.

Coolio. Thanks @pez! So @ericdallo & @arthurfucher rewrite-clj does happen to handle some technically incorrect code (ex. {:a}) but not yet things like mismatched or unclosed forms (`(not closed` [mismatch)). The performance aspect is also something to consider. Zippers aren't zippy. And mapping from editor char to rewrite-clj node is also a big concern.

This doesn't mean I won't take a stab at fixing up the rewrite-clj.paredit API (as it is currently designed) but it might not be a great fit (as it is currently designed) for paredit support for an editor.

I'm pretty ok with not supporting unbalanced parens for now, making things work for balanced seem more important right now, thanks

Good to know.

It might be a good idea to understand exactly what features you are trying to implement. Is it a complete set of paredit ops at arbitrary char positions?

We already support using clojure-lsp/rewrite-clj https://github.com/clojure-lsp/clojure-lsp-intellij/blob/lsp4ij/docs/features.md#paredit, making them bug-safe would be great already

what you see on those gifs are already using rewrite-clj, so from my POV, we just have few bugs in a working feature

Oh that's great to know. So you support specific paredit ops already using rewrite-clj but are just looking to fix bugs that are manifesting from rewrite-clj.

exactly

Okey dokey, I think (probably) my continued work on rewrite-clj paredit fixes could be of value to you (and they should be fixed anyway). I'm still unsure about char/node mismatch, but we can see how that plays out.

sounds great!

Also whitespace/formatting handling will probably be a thing, but again we can see how that plays out.

that we could fix with clojure-lsp format feature after applying the edit, currently we don't do that, but it's something it should not be hard (clojure-lsp call cljfmt format on the string after applying the edit)

yeah that might work, we'll see, I guess!

That’s what Calva does. 😃

Ah awesome to know, thanks @pez!

One nerdy thing I am very happy with is that in my wip paredit fixes, I've reworked unit tests to make it much easier to test and verify scenarios. The tests interpret the ⊚ char to be the current node position. So I can verify, for example, that a slurp forward on [⊚[] 1 2 3] results in the expected [[⊚1] 2 3]. This should make it easier for others to understand rewrite-clj paredit behaviour too.

That’s super helpful. We’re testing Calva’s structural editor in similar ways. Multi cursor aware even. 😀 And we have utilities in Calva to create such documents from the current editor state, and also to create an editor state from such documents.

(comment "expect `(get {}|) :a` to slurp to `(get {} :a)`"
           "but result is `(get {} ) :a`"
           (let [text "(get {}) :a"
                 row 1
                 col 8
                 pos {:row row, :col col, :end-row row, :end-col col}
                 zloc (-> text
                          (z/of-string {:track-position? true}))]
                (-> (z/find-last-by-pos zloc pos)
                    paredit/slurp-forward
                    z/root-string)))

(comment "expect `(get {} |) :a` to slurp to `(get {} :a)`"
           "but result is `(get {} ) :a`"
           (let [text "(get {} ) :a"
                 row 1
                 col 9
                 pos {:row row, :col col, :end-row row, :end-col col}
                 zloc (-> text
                          (z/of-string {:track-position? true}))]
                (-> (z/find-last-by-pos zloc pos)
                    paredit/slurp-forward
                    z/root-string)))

(comment "expect `(get {}| ) :a` to slurp to `(get {} :a)`"
           "And works!"
           (let [text "(get {} ) :a"
                 row 1
                 col 8
                 pos {:row row, :col col, :end-row row, :end-col col}
                 zloc (-> text
                          (z/of-string {:track-position? true}))]
                (-> (z/find-last-by-pos zloc pos)
                    paredit/slurp-forward
                    z/root-string)))

c/c @lee

Hi @arthurfucher, thanks for dropping by with a question. I'll take a look soon!

I had been working on a bunch of rewrite-clj paredit fixes but got distracted by other projects... I'll have to refresh my noggin...

okk! Let me know if you need anything else 😇

Before I go into your examples in detail, clarifications might help: • rewrite-clj row/col positions are 1-based • the rewrite-clj zipper movement granularity is the node To visualize with code:

(require '[rewrite-clj.paredit :as paredit]
         '[rewrite-clj.zip :as z])

;;                 12345678901  12
(->> (z/of-string "(get {}) :a\n:b" {:track-position? true})
     (iterate z/next)
     (take-while (complement z/end?))
     (mapv (fn [z] (let [[row col] (z/position z)]
                     {:row row :col col  :node (z/string z)}))))
;; => [{:row 1, :col 1, :node "(get {})"}
;;     {:row 1, :col 2, :node "get"}
;;     {:row 1, :col 6, :node "{}"}
;;     {:row 1, :col 10, :node ":a"}
;;     {:row 2, :col 1, :node ":b"}]

(for [col (range 1 11)
;;                             12345678901
      :let [zloc (z/of-string "(get {}) :a" {:track-position? true})
            find-pos [1 col]
            found-zloc (z/find-last-by-pos zloc find-pos)]]
  {:find-pos find-pos :found-pos (z/position found-zloc) :node (z/string found-zloc)})
;; => ({:find-pos [1 1], :found-pos [1 1], :node "(get {})"}
;;     {:find-pos [1 2], :found-pos [1 2], :node "get"}
;;     {:find-pos [1 3], :found-pos [1 2], :node "get"}
;;     {:find-pos [1 4], :found-pos [1 2], :node "get"}
;;     {:find-pos [1 5], :found-pos [1 5], :node " "}
;;     {:find-pos [1 6], :found-pos [1 6], :node "{}"}
;;     {:find-pos [1 7], :found-pos [1 6], :node "{}"}
;;     {:find-pos [1 8], :found-pos [1 1], :node "(get {})"}
;;     {:find-pos [1 9], :found-pos [1 9], :node " "}
;;     {:find-pos [1 10], :found-pos [1 10], :node ":a"})

;; row 1 col 8 might be an off by 1 error?
(-> (z/of-string "(get {}) :a" {:track-position? true})
    (z/find-last-by-pos {:row 1 :col 8}) ;; both map and vector syntaxes are supported for pos
    ((juxt z/position z/string)))
;; => [[1 1] "(get {})"]

(-> (z/of-string "(get {}) :a" {:track-position? true})
    (z/find-last-by-pos {:row 1 :col 7}) ;; both map and vector syntaxes are supported for pos
    ((juxt z/position z/string)))
;; => [[1 6] "{}"]

(-> (z/of-string "(get {}) :a" {:track-position? true})
    (z/find-last-by-pos {:row 1 :col 7}) ;; both map and vector syntaxes are supported for pos
    paredit/slurp-forward
    z/root-string)
;; => "(get {}) :a"

(-> (z/of-string "(get :xx) :a" {:track-position? true})
    (z/find-last-by-pos {:row 1 :col 7}) ;; both map and vector syntaxes are supported for pos
    paredit/slurp-forward
    z/root-string)
;; => "(get :xx :a)"

Thanks @lee, I'll take some time to read later today

FYI.. still working away at paredit cleanup. We've talked about slurping. Any concerns with barfing at this time?

Ok @arthurfucher, here's what I see for barf-forward. (reminder: (get []|) :a is not a possible node location) a) When not on the barfed element, we stay located in the sequence: [|:a :b :c] => [|:a :b] :c b) But if we are located at the barfed element, we retain our location at that element, but we have been barfed out of the sequence: [:a :b |:c] => [:a :b] |:c c) If we are at the space(s) node before barfed element, we are also barfed out [:a :b| :c] => [:a :b] |:c d) And if we are barfing out the last element (same as b, really, but also a reminder that you can't stay located in an empty sequence if that is what you were hoping for): [|:a] => [] |:a

Still plugging away at paredit fixes... got many issues resolved... I will probably try to tackle https://github.com/clj-commons/rewrite-clj/issues/317 next.

@lee I'm happy to help with the paredit fixes. Let me know if there's anything you'd like me to take on. (I work with @arthurfucher and @ericdallo)

Hi @andreribeirocamargo ! Thanks, I’m good for now. Once I get baseline fixes done, I’m gonna really benefit from feedback from usage.

Heya, if you want to help, I'd love some feedback on this issue: https://github.com/clj-commons/rewrite-clj/issues/339

yeah, the original issue mention also about the same issue with barfing (get [] |:a) becomes (get [])| :a, but I expected: (get []|) :a (get []| :a) becomes (get []|) :a as expected

hmmm... ok, I'll take a peek at that as well.

hmm actually @lee , looking again, I think the issue is about cursor position, and not about not doing barf. So, I think the issue could be in our side. I'll check with @andreribeirocamargo, so we can look at this

Sure, lemme know what you find! After I study barf a bit, I might ask you some questions.