Skip to content
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 support for jump-to-def for associations #373

Merged
merged 16 commits into from
May 30, 2024

Conversation

maxveldink
Copy link
Contributor

@maxveldink maxveldink commented May 8, 2024

Closes #358

Adds support for jump to definition for belongs_to, has_one, has_many and has_and_belongs_to_many associations on models.

@maxveldink
Copy link
Contributor Author

I have signed the CLA!

@andyw8 andyw8 added the railsconf-hackday Issues intented for working on during RailsConf 2024 Hack Day label May 8, 2024
@andyw8
Copy link
Contributor

andyw8 commented May 15, 2024

👋 Looking into the on_class_node_enter problem.

@andyw8
Copy link
Contributor

andyw8 commented May 15, 2024

Ah, we don't need to listen for class node events, we can use the nesting:

model_name: @nesting.join("::")

@andyw8
Copy link
Contributor

andyw8 commented May 15, 2024

I notice the Builder classes (example) are marked :nodoc: which usually means they should treated as internal, so we may need to add a public interface for that in Rails.

@maxveldink maxveldink marked this pull request as ready for review May 21, 2024 13:14
@maxveldink maxveldink requested a review from a team as a code owner May 21, 2024 13:14
@maxveldink maxveldink requested review from andyw8 and st0012 May 21, 2024 13:14
@maxveldink
Copy link
Contributor Author

Thanks @andyw8! I'm flipping this to ready since it's about good from my end. Ready to review whenever!

How do I go about upstreaming the behavior we need for the Builder classes? Can we merge this with the private interfaces if we have the next steps for a better interface?

Comment on lines 143 to 149
rescue NameError => e
{
result: {
error: e.message,
},
}
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the right behavior here; I opted to return the error message; but I think would could return a nil result just like when the model name doesn't resolve properly. Thoughts?

Copy link
Contributor

@andyw8 andyw8 May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think returning nil is more appropriate here.

Thinks make me think though: Sometimes it's confusing for the user if they try something and the editor seems to do nothing. Perhaps there could be an info response, which prints something to the log in these kinds of situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched it to nil for now. I like the idea of having info responses print to the log. Should we add an issue to add that support?

@andyw8
Copy link
Contributor

andyw8 commented May 21, 2024

Nice!

I found there's a simpler way than using the Builder classes - reflect_on_association It will also work with options, e.g. class_name, and it's documented so there's no concerns about using private APIs.

I branched off your PR and added a couple of commits in this branch:

https://github.com/Shopify/ruby-lsp-rails/compare/andyw8/go-to-def-assoc

If you want to update your branch then go ahead, otherwise I can things over from here.

@maxveldink maxveldink changed the title WIP: Add support for jump-to-def for associations Add support for jump-to-def for associations May 22, 2024
@maxveldink maxveldink requested a review from andyw8 May 22, 2024 10:29
@maxveldink
Copy link
Contributor Author

@andyw8 Thanks! I was looking for a more stable way too and this works great.

I pulled in your commits and cleaned up the PR some more. Should be good for another look

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments. This looks great!

lib/ruby_lsp/ruby_lsp_rails/definition.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/ruby_lsp_rails/definition.rb Outdated Show resolved Hide resolved
Co-authored-by: Vinicius Stock <vinistock@users.noreply.github.com>
@maxveldink maxveldink requested a review from vinistock May 29, 2024 08:56
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but linting is failing

@maxveldink
Copy link
Contributor Author

@vinistock Ahh yes, GH PR additions always get me. Should be good to go 👍🏻

@andyw8
Copy link
Contributor

andyw8 commented May 29, 2024

btw we're starting on fixing the target behaviour so you'll be to right click on the symbol rather than the has_many (etc.): Shopify/ruby-lsp#2099

@andyw8 andyw8 enabled auto-merge (squash) May 30, 2024 20:09
@andyw8 andyw8 merged commit 0c85e7a into Shopify:main May 30, 2024
21 checks passed
@maxveldink maxveldink deleted the mv/go-to-def-associations branch May 31, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
railsconf-hackday Issues intented for working on during RailsConf 2024 Hack Day
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide jump-to-definition for associations
3 participants