Folding Range LSP support #615
Conversation
kv9898
force-pushed
the
feature/foldable-comments
branch
from
3528daa to
573606b
Compare
kv9898
force-pushed
the
feature/foldable-comments
branch
from
573606b to
cfd0626
Compare
|
@lionel- is there anything else I can help to do? |
|
@kv9898 We just need to get some time to look at this. Probably next week. |
Thank you! I was worried that you were working on some breaking changes that would cause a merge conflict. It's a little bit difficult to rebase everything at this stage.😄 |
There was a problem hiding this comment.
Hi @kv9898. Thanks a lot for looking into this and I apologise for only taking a look at this now.
I think parsing for folding ranges need to be done by walking down the syntax tree. We need that structure to properly assess whether a character is indeed a delimiter or if it's something else. For instance in "foo}" the } character is not a delimiter.
As you consider reworking this PR, please know that we plan for LSP functionality to move to https://github.com/posit-dev/air/. In that project, we use Rowan (see https://github.com/rust-analyzer/rowan but we use Biome's fork) to represent the syntax tree instead of Tree-Sitter (we still parse with tree-sitter for now but convert the tree to Rowan). This means that if you do write a folding range algorithm on top of the TS tree in Ark, it will eventually have to be rewritten on top of Rowan (and/or the typed AST we generate, see https://github.com/posit-dev/air/blob/0.1.0/crates/air_r_syntax/src/generated/nodes.rs).
The advantage of implementing this in Air instead of Ark is that the functionality will be available from other editors like VS Code, Vim, Zed. And in Positron it will also be available when the R session is shut down.
If you'd like we could go over Air internals together in a pairing session. I think you're in the UK timezone? I'm in CET. We use the Zed editor to collaborate btw. My email is lionel à posit.co if you'd like to arrange that.
| @@ -0,0 +1,299 @@ | |||
| use regex::Regex; | |||
There was a problem hiding this comment.
Needs a Posit copyright header as in the other files.
| /// Detects and returns folding ranges for comment sections and curly-bracketed blocks | ||
| pub fn folding_range(document: &Document) -> anyhow::Result<Vec<FoldingRange>> { | ||
| let mut folding_ranges: Vec<FoldingRange> = Vec::new(); | ||
| let text = &document.contents; // Assuming `contents()` gives the text of the document |
There was a problem hiding this comment.
| let text = &document.contents; // Assuming `contents()` gives the text of the document | |
| let text = &document.contents; |
| for (char_idx, c) in line_text.char_indices() { | ||
| match c { | ||
| '{' | '(' | '[' => { | ||
| bracket_stack.push((line_idx, char_idx)); | ||
| comment_stack.push(Vec::new()); | ||
| }, | ||
| '}' | ')' | ']' => { | ||
| (folding_ranges, comment_stack) = |
There was a problem hiding this comment.
Matching on characters like this will work incorrectly with delimiter characters in strings for instance.
There was a problem hiding this comment.
Ohh I see! In this case it is indeed better to do it with a syntax tree.
It seems that the call for folding ranges has been quite a while: posit-dev/positron#18, posit-dev/positron#2924, posit-dev/positron#3822.
I was initially only thinking about adding foldable comments, but it seems that doing this will disable the existing folding support for things like regions and brackets. Therefore, I rewrote these functionalities also.
The PR already supports the folding-range-handling of brackets, regions, code cells and nested comment sections:
For the handling of regions and code cells, I'm currently implementing a simple lookup of regex, which respects the existing behaviour and disregards any nested structures. I note that this kinds of get messy when you are also using the nested comments. But maybe that's a separate issue?