-
Notifications
You must be signed in to change notification settings - Fork 146
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
Definition support for autoloaded constants #1995
Definition support for autoloaded constants #1995
Conversation
I have signed the CLA! |
I have signed the CLA! |
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.
This is looking great! There's only one piece missing. To improve the precision of where you can cmd click, you need to add one extra check to this condition
target.name != :autoload
connected another email with the CLA |
I have signed the CLA! |
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.
Needs a rebase, otherwise looks ready to go.
@@ -145,6 +147,18 @@ def handle_require_definition(node) | |||
end | |||
end | |||
|
|||
sig { params(node: Prism::CallNode).void } | |||
def handle_autoload_definition(node) | |||
|
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.
Let's remove the blank line here.
There is one typecheck failure, it may indicate a missing nil check. |
|
…n.rb#handle_autoload_definition
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.
Did another pass and this looks ready to go. We just need to rebase and we should be good to merge.
Sorry, the development on the LSP is moving fast and I feel like you keep chasing a moving target. We made changes to allow for more precise jump to definition based on a method call and its arguments. You can see an example for require definition. The idea is that the event now matches You will need to do the exact same thing, but for |
@Super-Xray Thanks for the PR! Because we want to have autoload's definition support to be released soon-ish to match other code navigation features we made recently, I took the liberty to open #2395 with your commits and built some small improvements on top of it. (I tried to merge this PR first by resolving the conflicts, but then I found some tests need to be updated as well to account for some recent changes. Since GH UI doesn't allow me to update non-conflicting files, nor can I push to your fork, I needed to open another PR instead). |
Superseded by #2395 |
Motivation
There are an issue calling for complete this function (#1893).
Implementation
autoload
argument is aPrism::CallNode
which has name:autoload
. I createhandle_autoload_definition
method and add it toon_call_node_enter
method in 'lib/ruby_lsp/listeners/definition.rb'. Then inhandle_autoload_definition
method, we can use the name of the constant in thePrism::SymbolNode
under thePrism::CallNode
and pass the name tofind_in_index
, which can create the response of jumping.Automated Tests
Completed automated tests in
test/requests/definition_expectations_test.rb
, includingtest_jumping_to_autoload_definition_when_declaration_exists
,test_jumping_to_autoload_definition_with_two_definition
andtest_does_nothing_when_autoload_declaration_does_not_exist
Manual Tests
.rb
file which hasautoload
arguments.autoload
arguments, press ctrl & click it