-
Notifications
You must be signed in to change notification settings - Fork 30
Add Definition support for routes #331
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ def activate(global_state, message_queue) | |
@global_state = T.let(global_state, T.nilable(RubyLsp::GlobalState)) | ||
$stderr.puts("Activating Ruby LSP Rails addon v#{VERSION}") | ||
# Start booting the real client in a background thread. Until this completes, the client will be a NullClient | ||
Thread.new { @client = RunnerClient.create_client } | ||
Thread.new { @client = RunnerClient.create_client }.join | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (corrected in #356) |
||
end | ||
|
||
sig { override.void } | ||
|
@@ -84,7 +84,7 @@ def create_document_symbol_listener(response_builder, dispatcher) | |
end | ||
def create_definition_listener(response_builder, uri, nesting, dispatcher) | ||
index = T.must(@global_state).index | ||
Definition.new(response_builder, nesting, index, dispatcher) | ||
Definition.new(@client, response_builder, nesting, index, dispatcher) | ||
end | ||
|
||
sig { params(changes: T::Array[{ uri: String, type: Integer }]).void } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,25 +11,35 @@ module Rails | |
# | ||
# Currently supported targets: | ||
# - Callbacks | ||
# - Named routes (e.g. `users_path`) | ||
# | ||
# # Example | ||
# | ||
# ```ruby | ||
# before_action :foo # <- Go to definition on this symbol will jump to the method if it is defined in the same class | ||
# ``` | ||
# | ||
# Notes for named routes: | ||
# - It is available only in Rails 7.1 or newer. | ||
# - Route may be defined across multiple files, e.g. using `draw`, rather than in `routes.rb`. | ||
# - Routes won't be found if not defined for the Rails development environment. | ||
# - If using `constraints`, the route can only be found if the constraints are met. | ||
# - Changes to routes won't be picked up until the server is restarted. | ||
andyw8 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class Definition | ||
extend T::Sig | ||
include Requests::Support::Common | ||
|
||
sig do | ||
params( | ||
client: RunnerClient, | ||
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::Location], | ||
nesting: T::Array[String], | ||
index: RubyIndexer::Index, | ||
dispatcher: Prism::Dispatcher, | ||
).void | ||
end | ||
def initialize(response_builder, nesting, index, dispatcher) | ||
def initialize(client, response_builder, nesting, index, dispatcher) | ||
@client = client | ||
@response_builder = response_builder | ||
@nesting = nesting | ||
@index = index | ||
|
@@ -43,8 +53,19 @@ def on_call_node_enter(node) | |
|
||
message = node.message | ||
|
||
return unless message && Support::Callbacks::ALL.include?(message) | ||
return unless message | ||
|
||
if Support::Callbacks::ALL.include?(message) | ||
handle_callback(node) | ||
elsif message.end_with?("_path") || message.end_with?("_url") | ||
handle_route(node) | ||
end | ||
end | ||
|
||
private | ||
|
||
sig { params(node: Prism::CallNode).void } | ||
def handle_callback(node) | ||
arguments = node.arguments&.arguments | ||
return unless arguments&.any? | ||
|
||
|
@@ -62,7 +83,21 @@ def on_call_node_enter(node) | |
end | ||
end | ||
|
||
private | ||
sig { params(node: Prism::CallNode).void } | ||
def handle_route(node) | ||
result = @client.route_location(T.must(node.message)) | ||
return unless result | ||
|
||
file_path, line = result.fetch(:location).split(":") | ||
|
||
@response_builder << Interface::Location.new( | ||
uri: URI::Generic.from_path(path: file_path).to_s, | ||
range: Interface::Range.new( | ||
start: Interface::Position.new(line: Integer(line) - 1, character: 0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in |
||
end: Interface::Position.new(line: Integer(line) - 1, character: 0), | ||
), | ||
) | ||
end | ||
|
||
sig { params(name: String).void } | ||
def collect_definitions(name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,11 @@ | ||
# frozen_string_literal: true | ||
|
||
class ApplicationController < ActionController::Base | ||
def create | ||
user_path(1) | ||
user_url(1) | ||
users_path | ||
archive_users_path | ||
invalid_path | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# typed: true | ||
# frozen_string_literal: true | ||
|
||
# Route source locations are normally only available in development, so we need to enable this in test mode. | ||
ActionDispatch::Routing::Mapper.route_source_locations = true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
Rails.application.routes.draw do | ||
resources :users do | ||
get :archive, on: :collection | ||
end | ||
end |
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 seems that the behaviour added in https://github.com/rails/rails/pull/47877/files is not working properly with Ruby head. Since Rails itself doesn’t test against Ruby head, I think it’s reasonable if we don’t either, and wait until this is fixed upstream.