-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add code lens for mapping actions to routes #241
Conversation
1cd5fe8
to
0ee22a6
Compare
@_response << create_code_lens( | ||
node, | ||
title: [verb, path].join(" "), | ||
command_name: "rubyLsp.openFile", |
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 kind of silly to me that there's no native support in editors for a file opening command (that I can see). Is there a way around this, or should I proceed with my PR that implements this over on the extension? There's a 'editor.action.showReferences'
command, but I can't seem to get the expected parameters to work using it. I also don't know if all LSP supported editors have this command available, or just vscode?
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 had a patch like this coming this week. I don't work for Shopify and have been really busy at work so I didn't have time to get the tests done yet, so I wasn't pushing it up.
The format I had for routes was:
name: route.name,
verb: route.verb,
path: route.path.sub(/...format./, ""),
controller: route.controller,
action: route.action,
source_location: "#{::Rails.application.root}/{route.source_location.sub(":", "#L")}",
views: ActiveSupport::Inflector.safe_constantize((route.controller.to_s + "_controller").camelize).new.lookup_context.find_all("#{route.controller}/#{route.action}").map do |route|
{ filename: route.identifier, format: route.format }
end,
}
If you use vscode.open, you can do this for VS Code at least.
If you want to do it all you could use just the uri for non vscode editors (but I haven't checked on how to do this yet).
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.
Can you please include the views as I have code action coming to open corresponding views?
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.
Also note, if you use #L#{line_number} you can jump to the right line.
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.
View lookup was another feature I was thinking of too, but I didn't get that far. I was thinking it would be a separate code lens, and maybe an entirely different request to the rack app. I'll see if I have time to implement it over the next few weeks, but I can't promise anything right now.
I'm not directly part of the team maintaining this repo, so let's get their opinions in this patch and the views idea as well before we proceed. If we do end up using some of these ideas, I'll give you credit via co-author on what I commit.
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.
Either way I will be doing the views. I am trying to replicate most of Rubymine for my team so I will probably do an add-on if needed.
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.
Thanks for the co-authorship :)
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.
From looking at the spec, a Code Lens doesn't need to have a command associated with it, so we could ship this with only the label, and deal with the command behaviour in a follow-up PR.
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.
@rickcaudill thanks for those suggestions. I think we can handle it separately, so I've created #384 to track.
6f27dd2
to
2daf932
Compare
Looks like this is failing on HEAD, but I don't know why yet. |
When speaking with Jean and Aaron previously, they suggested that if necessary we can make modifications to Rails to expose an API for these kinds of things. |
b5aace7
to
69fcf97
Compare
I forgot about this when adding #331. There's a bit of commonality, so once that's merged we can hopefully rework this.
Which aspect do you mean here? |
Updated due to the changes in |
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
private | ||
|
||
def generate_code_lens_for_source(source) | ||
with_server(source) do |server, uri| | ||
sleep(0.1) while RubyLsp::Addon.addons.first.instance_variable_get(:@client).is_a?(NullClient) |
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.
We now have Shopify/ruby-lsp#2216 so we can simplify this a little:
sleep(0.1) while RubyLsp::Addon.addons.first.instance_variable_get(:@client).is_a?(NullClient) | |
sleep(0.1) while RubyLsp::Addon.get("ruby-lsp-rails").is_a?(NullClient) |
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.
Sure, done.
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 isn't released yet, so I'll go ahead and revert.
1bf5dfb
to
33b856f
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.
Looks great! Let's get @vinistock to review too though.
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
831fe27
to
6883e7f
Compare
Allows us to map controller actions to routes. Co-authored-by: Gannon McGibbon <gannon.mcgibbon@shopify.com>
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
To support mapping controller actions to routes in Shopify/ruby-lsp-rails#241.
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.
Thank you! This is great
Part of #50
Adds code lens to find routes in the routes file based on a given controller action. You need to grep bin/rails routes for this information manually. If Rails already knows this info, we should make it more easily accessible.