-
Notifications
You must be signed in to change notification settings - Fork 17
Emit functions passed as named arguments as document symbols #858
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
base: feature/testthat-symbols
Are you sure you want to change the base?
Conversation
1a0d3de
to
698f1a3
Compare
698f1a3
to
f67b10f
Compare
68df2ed
to
1b58bb0
Compare
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've looked over the code and, with the caveat that I'm still a relative newcomer to ark, LGTM.
My main reaction is: I went to the tests to get a feel for what really happens here in the symbols area of the LSP and what is changing in this PR. But it's pretty hard for a human to parse the expectations that use the insta::assert_debug_snapshot!()
mechanism.
Is there a reason the changes in this PR are tested only that way? Versus the more concrete assertions in some of the other tests. I'm definitely thinking about my recent foray into completions and tests, where it was possible to check that a certain completion was present (and of a specific form) or absent.
It's just for convenience, of both writing the tests and updating them (i.e. if you insert a line all ranges change). |
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.
for arg in arguments.children(&mut cursor) { | ||
if arg.kind() != "argument" { |
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.
Maybe walk this for clarity arguments.children_by_field_name("argument", &mut cursor)
// that might contain functions, etc. | ||
collect_symbols(&arg_value, contents, 0, symbols)?; | ||
|
||
if arg_value.kind() == "function_definition" { | ||
if let Some(arg_fun) = arg.child_by_field_name("name") { | ||
// If this is a named function, collect it as a method | ||
collect_method(&arg_fun, &arg_value, contents, symbols)?; | ||
} else { | ||
// Otherwise, just recurse into the function | ||
let body = arg_value.child_by_field_name("body").into_result()?; | ||
collect_symbols(&body, contents, 0, symbols)?; | ||
}; | ||
} |
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.
It feels like maybe the collect_symbols()
call up top should be mutually exclusive with the function_definition
branch?
Like maybe you should first match arg_value.kind()
with a variant for "function_definition"
and then a fallthrough of collect_symbols()
?
Branched from #856
Addresses posit-dev/positron#6546
Addresses posit-dev/positron#6107 and posit-dev/positron#5241
The main functionality this PR aims to implement is inclusion of R6 symbols in the outline.
I think it makes sense to include all functions passed as named arguments, to any calls and not just
R6Class
. This is in contrast to workspace symbols reachable across files. In this case I think only R6 methods should be registered as workspace symbols (see posit-dev/positron#6549). In general document symbols are more exhaustive than workspace one, offering access to local objects.Changes:
We now collect symbols in the RHS of assigned objects. In particular this means we get a chance to collect methods in the
r6class()
call above. This also means we'll collect symbols in cases like this, which I think makes sense:We now collect symbols across call arguments. This allows us to collect methods nested deeply inside the
r6class()
call. This also means we also collect symbols in cases like the following, which I also think makes sense:Addresses R code outline not working in nested control flow elements positron#6107 and Ark: Blocks in function calls should be included in outline positron#5241
Finally, functions passed as named arguments are collected as Method symbols. Previously only functions assigned with
<-
at top level or in{}
would be collected.QA Notes:
All these changes are tested in the backend. On the frontend you should now see the outline/breadcrumbs filled with
initialize
,foo
, andbar
methods.Screen.Recording.2025-06-27.at.12.36.09.mov