-
Notifications
You must be signed in to change notification settings - Fork 20
Recurse into special constructs to find document symbols #892
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions requesting clarifying behavior and a few more tests but I think this looks fairly straightforward?
crates/ark/src/lsp/symbols.rs
Outdated
NodeType::BracedExpression => { | ||
let old = ctx.top_level; | ||
ctx.top_level = false; | ||
collect_list_sections(ctx, node, contents, current_level, symbols)?; | ||
ctx.top_level = old; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change the behavior of general braced expressions? i.e.
# a section ----
{
# another one ----
}
And do we have tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! We tested the brace-in-call and brace-in-function cases, but not the brace-in-program case.
I've tweaked a snapshot to include these (as well as the brace-in-brace case) and regenerated before all changes in this PR, then reran the tests with all the changes (including removed current_level
), and there were no snapshot changes.
crates/ark/src/lsp/symbols.rs
Outdated
// Only collect sections | ||
return Ok(()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this comment means. Does it mean no recursion? Can you provide more details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep no recursion, the comment is now clearer about that
crates/ark/src/lsp/symbols.rs
Outdated
ctx, | ||
&node, | ||
contents, | ||
0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmmmm 0
? Are you sure you shouldn't pass the current_level
through to here? You do for the function body. Add a test either way about this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or at least a comment here about why its 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant 0 here, but I think you're right to be confused. With the changes made over time, we no longer propagate the current level across syntax constructs. So I've just removed this argument.
5d654e0
to
ce3fb52
Compare
Addresses posit-dev/positron#8881
We now consistently recurse inside these constructs to collect nested document symbols such as comment sections:
The behaviour regarding nested assignments implemented in #859 has been adapted so that
{
blocks in if-else branches and loops still counts as top-level. This way these assignments are all treated as consistently part of the outline:In addition we also now collect comment sections in parameter lists of function definitions:
QA Notes
Comment sections and other kinds of document symbols in if-else branches and loops are now part of the outline:
Comment sections in parameter lists of function definitions are now included as well. We don't recurse through default arguments though.