BetterThanTomorrow / calva

Clojure & ClojureScript Interactive Programming for VS Code
https://marketplace.visualstudio.com/items?itemName=betterthantomorrow.calva
Other
1.66k stars 217 forks source link

Slurp forward to wrap a form introduces extra space #2085

Open wevre opened 1 year ago

wevre commented 1 year ago

With cursor right before a form, if I type a delimiter and then slurp forward, there is a space between the opening delimiter and the newly wrapped form.

|:b
;; Want to wrap :b, type '['
[|]:b
;; slurp forward
[| :b]
;;^ don't like that space, wish instead it were
[|:b]

This outcome is the same regardless of delimiter.

Request that Calva not include this extra preceding space.

PEZ commented 1 year ago

Thanks. This has to do with how we format (or not) the forms after Paredit operations. It was an effect of starting to use the formatter in on-type mode. In on-type mode our formatter relaxes the formatting and does not trim spaces.

I don't know what the real solution is here, but I think it makes sense with on-type by default and maybe looking over which Paredit operations we want to either do some other post-formatting on, or if they should be changed in the actual paredit operation. Like with the example, the slurp replaces the bracket with a space, maybe it could delete the bracket instead, making on-type post formatting sufficient...

Something you would want to take a look at, @SillyCoon?

I think it is worth having a look at all paredit operations https://calva.io/paredit/ and see how they behave. It could give us an idea of what we should do about it, generally. Then maybe use TDD to expose the cases we want to change, and fix them.

SillyCoon commented 1 year ago

I'd be happy to take a look, thank you for the suggestion @PEZ!

SillyCoon commented 1 year ago

Hi @PEZ. After a little investigation, I guess the issue here is that we don't check if the 'slurping' form is empty or not and it just inserts the whitespace to split the args like:

[a|] b => (a b) is correct but [|] b => ( b) behaves the same

I don't really know if should we include the formatting after slurp forward or after the slurp backward, but in general it could fix a lot of bugs without actually fixing them.

PEZ commented 1 year ago

Thanks for checking this out, @SillyCoon!

I don't really know if should we include the formatting after slurp forward or after the slurp backward, but in general it could fix a lot of bugs without actually fixing them.

Iirc we are doing formatting, but we do a relaxed variant, which does not trim whitespace inside brackets. We used to do the trimming too, but that had other effects, #1924 might have some clues about it. I think Slack might hold some discussion that led to #1924.

It could be that we should go back to how we used to do it. Idk... Did you test the other paredit commands to see if there are more places where the trimming should be done? If slurp is the only instance, we can do something special there maybe.

SillyCoon commented 1 year ago

Sorry, my bad. I missed the important point in #1924 and the discussion in Slack about the formatting for Paredit 😢, but now I understand. I definitely should check Slack more often 😄

I found at least another 3 cases with "strange" behavior (IMO) for Barf and Slurp.

https://recordit.co/G8SDOUbRiK https://recordit.co/Pkudpa9bj2 https://recordit.co/aUq1vB2iI9

So yeah, probably more cases like this exist. Unfortunately, I don't have time to check the other commands now. But I didn't find any other complaints about spacing in Slack and Paredit issues. Soooooooo, I too don't know how to handle it correctly. Since I found the root cause I can just fix this specific case, but there are people who like the existing behavior (https://clojurians.slack.com/archives/CBE668G4R/p1676869762860819?thread_ts=1676577580.743059&cid=CBE668G4R)

PEZ commented 1 year ago

there are people who like the existing behavior

There will always be people with differing tastes here. We'll have to remove the feature to not implement it in a way that someone does not fancy. 😄

Thanks for the recordings. However, very hard to discuss the cases when they are in some other place. I prefer text representations that I can read here and copy and paste and play with.

That said:

I think all three will get correct if we merge this config to on-type config when barfing/slurping:

{:remove-surrounding-whitespace? true
 :remove-trailing-whitespace? true
 :insert-missing-whitespace? true}

Note that the third recording, is a special case that we might not reach with formatting, because it happens on the top level and the formatter, as we use it today, does not always work there. We can skip that special case for now, and make it behave when happening inside a form.

SillyCoon commented 1 year ago

Hi @PEZ, sorry for the recordings. I will attach the text then, I like this format more too. I checked all the other Paredit edit operations and they work ok, the only thing that I found is this:

(()| :b)

Split Sexp

Result: (:a)( :b)

Should be: (:a)(:b)

mkreis commented 6 months ago

Seems like this is a duplicate / subset: https://github.com/BetterThanTomorrow/calva/issues/1440