Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Primitive functionality to display comment sections in outlines #571

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

kv9898
Copy link
Contributor

@kv9898 kv9898 commented Oct 6, 2024

Attempt to (partly) fix posit-dev/positron#3822

image

I've managed to make comments which are in the shape of "## Title ####/----/====" appear in the outline with the changes in this pull request, but I struggled to make them appear in a hierarchical form/foldable.

@kv9898 kv9898 marked this pull request as ready for review October 6, 2024 21:44
Copy link
Contributor

@lionel- lionel- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look at this!

Regarding folding, see @juliasilge's comment in posit-dev/positron#4174 (comment). Have you looked at https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_foldingRange?

But I wonder if we can provide ranges both from the rules in our extension's json config, and from the LSP?

In any case, even without ranges it seems like a good progress on this issue.

Comment on lines 176 to 184
NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment)
| NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment)
) {
match index_assignment(node, contents, parent, symbols) {
Ok(handled) => {
if handled {
return Ok(true);
}
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please make sure you're using +nightly for rustfmt? See

"rust-analyzer.rustfmt.extraArgs": ["+nightly"],

}
}

// Existing code for indexing assignments or other nodes...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Existing code for indexing assignments or other nodes...

@@ -120,29 +120,73 @@ fn is_indexable(node: &Node) -> bool {
true
}

// Function to parse a comment and return the section level and title
fn parse_comment_as_section(comment: &str) -> Option<(usize, String)> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add some tests for this function?


None
}

fn index_node(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are needed for this function too. You can do this to get a Rope and node:

let code = "my_code";
let document = Document::new(code, None);
let node = ast.root_node();

@kv9898
Copy link
Contributor Author

kv9898 commented Oct 7, 2024

Hi @lionel-, thank you so much for the suggestions!! I have removed the line as per your suggestion and reformatted the code using +nightly. However, I'm really inexperienced with Rust (this is actually my first time writing Rust and I extensively used ChatGPT for this).

I'm a bit struggling with the other suggestions. I'm not sure where do add the tests and how to add the the folding functionality. I have allowed edits by maintainers and would really appreciate if you can help/guide me through this because I personally really want this outline/folding functionality to work in Positron :)

@lionel-
Copy link
Contributor

lionel- commented Oct 9, 2024

@kv9898 No worries, I've added a couple tests, feel free to add more from these examples.

@kv9898
Copy link
Contributor Author

kv9898 commented Oct 10, 2024

@lionel- Thank you so much for helping me out! I'm pretty happy with how it looks now!

crates/ark/src/lsp/symbols.rs Outdated Show resolved Hide resolved
crates/ark/src/lsp/symbols.rs Outdated Show resolved Hide resolved
crates/ark/src/lsp/symbols.rs Outdated Show resolved Hide resolved

None
}

fn index_node(
node: &Node,
contents: &Rope,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta comment

Looks like our indexer, which watches the whole "workspace" (i.e. multiple folders of files), also does something similar. i.e. see index_comment() there.

That indexer powers symbols() in this file, which powers workspace symbols. i.e. when you hit CMD + T then it pops up all the symbols in the workspace - even in closed files.

Screenshot 2024-10-11 at 3 17 20 PM

What we are changing here powers document symbols, i.e. symbols within a single document. This powers the Outline editor view we have been talking about, and it powers this Outline quick pick

Screenshot 2024-10-11 at 3 19 04 PM

It will eventually probably be good to merge all the infrastructure here. Ideally we'd have one set of code that we can run on a file that returns all its symbols, and then we map them into either a workspace symbol or document symbol (its essentially indexer::index_file() i think, but it doesn't know how to create nested outlines, which is important).

I don't think this should block anything, but we should keep this in mind and open an issue to merge all of this code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds very desirable! I would really appreciate the nested outline and foldable comment functionalities. Sadly, as a student primarily working on R-based data analysis projects, I'm new to Rust and unfamiliar with the ark and LSP architecture, I'm struggling to carry this out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can take care of it!


let symbol = DocumentSymbol {
name: title, // Use the title without the trailing '####' or '----'
kind: SymbolKind::STRING, // Treat it as a string section
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that at the top of this file in symbols() we represent Sections with SymbolKind::MODULE.

I do think SymbolKind::STRING looks a little better, so could you please change MODULE to STRING to keep them in sync?

Comment on lines +124 to +134
fn parse_comment_as_section(comment: &str) -> Option<(usize, String)> {
// Match lines starting with one or more '#' followed by some non-empty content and must end with 4 or more '-', '#', or `=`
// Ensure that there's actual content between the start and the trailing symbols.
if let Some(caps) = indexer::RE_COMMENT_SECTION.captures(comment) {
let hashes = caps.get(1)?.as_str().len(); // Count the number of '#'
let title = caps.get(2)?.as_str().trim().to_string(); // Extract the title text without trailing punctuations
return Some((hashes, title)); // Return the level based on the number of '#' and the title
}

None
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the function does return the number of hashes (level of comment). I wanted this to help with the foldable comments/nested outlines. But I struggled to get this to work.

@lionel- lionel- merged commit abcabe9 into posit-dev:main Oct 14, 2024
1 check passed
@lionel-
Copy link
Contributor

lionel- commented Oct 14, 2024

Thanks a lot @kv9898!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outline view: Support nested sections in R scripts
3 participants