helix-core/surround: Syntax aware
#6893
Conversation
Prior to this change, attempting to replace the double quotes in `" asdf "` would fail with "Cursor on ambiguous surround pair". Now, after running e.g. `:set-language rust` in a file with the contents `" asdf "`, it is now possible to replace the double quotes with `mr` as you could with any other pair. This change essentially makes any pair that you can use `mm` on to go to the matching pair also able to be replaced with `mr` or deleted with `md` (whether they be built-in, or provided by a tree-sitter grammar).
|
This is a great change, I run up against this all the time. A much more robust solution than what vim plugins tend to do! |
|
For testing you can use the integration test framework which runs a full editors instance. You can then simply invoke the appropriate keybindings and compare the resulting text. An easy to understand example might be |
|
I'll try to whip something up shortly. |
|
you don't need podman/docker for the integration tests AFAIK |
|
Yeah, you're 100% right, I was thinking of another project I had once hacked on 😅 Sorry for the confusion; I'll gladly write a test regardless! |
|
OK, integration tests added in da73d79! Everything passes for me. |
There was a problem hiding this comment.
This is nice improvement.
However there can be inconsistencies between what TS returns and the simple textual matching. I think only using TS in such a special case is too much of a hack. TS based matching works fundamentally differently than textual matching. It would be weird if MD" would work differently (better) if the " was selected than if the cursor was inside of the match.
Instead I wonder whether we should change the pair matching to always use tree sitter when available. This would work much better with strings and escapes. For example currently while foo is selected in (foo, ")") and using mr([ would yield [foo, "]") instead if [foo, ")" ].
Implementation wise you would probably need to add some custom lgoic tough. Basically asecndjng the TS tree from the current cursor and stopping at the first node that start and end with thendesirted start/close character. (For each n you would go up one extra layer)
|
|
||
| let match_pos = find_matching_bracket(syntax, &text.into(), pos) | ||
| .ok_or(Error::CursorOnAmbiguousPair)?; | ||
| let close_char = text.char(match_pos); |
There was a problem hiding this comment.
I don't think we should be reading the close_char again here. If we run into some edgecase where find_matching_bracket does not return the same close char we should just return an error. Everything else would be unintuitive.
|
I think I get what you mean. I'll do some playing, and probably end up asking some questions in the Matrix room. My TS-fu is non-existent, so the work will be slow going, but I'll try to keep this PR apprised of my current progress. (I'll likely end up making the history ugly in the process, but I'll squash it once it appears to work.) |
|
What is needed to resurrect this? |
| // which side of the char we should be searching on. | ||
| let syntax = syntax.ok_or(Error::CursorOnAmbiguousPair)?; | ||
|
|
||
| let match_pos = find_matching_bracket(syntax, &text.into(), pos) |
There was a problem hiding this comment.
This line is the source of the current merge conflict. The RopeSlice can be passed on through now.
| let match_pos = find_matching_bracket(syntax, &text.into(), pos) | |
| let match_pos = find_matching_bracket(syntax, text, pos) |
Prior to this change, attempting to replace the double quotes in
" asdf "would fail with "Cursor on ambiguous surround pair".Now, after running e.g.
:set-language rustin a file with the contents" asdf ", it is now possible to replace the double quotes withmras you could with any other pair.This change essentially makes any pair that you can use
mmon to go to the matching pair also able to be replaced withmror deleted withmd(whether they be built-in, or provided by a tree-sitter grammar).I would like to add tests for this, but I don't know how I should go about doing this (specifically, creating e.g. a Rust tree-sitter
Syntaxinstance). I noticed one instance inhelix-core/tests/indent.rs, but.... it seemed kinda heavy-weight. I'd be happy to use that approach, but I'm hoping to learn about something that's less verbose. Suggestions / examples appreciated :)