Add scope to list of symbols

[natematykiewicz]

Fixes #311

[natematykiewicz]

I have signed the CLA!

[natematykiewicz]

I just rebased this to resolve the merge conflict

[andyw8]

I should have suggested this in https://github.com/Shopify/ruby-lsp-rails/pull/332 too, but can we add an example of this to the dummy app to allow for easy manual testing?

[natematykiewicz]

Done! https://github.com/Shopify/ruby-lsp-rails/pull/333/commits/0630ff5be0d7b1be6b42a87757a575faf5470cc5

[andyw8]

We need to change the behaviour a little:

For before_create, it's expected there will be two entries, since you can give it a list of callbacks. It would be the equivalent of writing:

before_create :foo
before_create -> () {}

But scope is different, the first argument is the name and second argument is the relation to be applied, so we should only see one entry in the explorer, i.e. scope :adult.

[natematykiewicz]

@andyw8 Ahh, right. I haven't figured out how to run this locally yet, but the test output seems to do what you're looking for now. https://github.com/Shopify/ruby-lsp-rails/pull/333/commits/510342c13665fc285ef6149c84879d299dfc7692

[andyw8]

This might not be a blocker, but when testing I realised that we will be unintentionally listing scope entries in routes.rb.

[andyw8]

@vinistock @st0012 we could try make this only detect scope for ActiveRecord instances, but then we'd also have deal with cases such as scopes being defined in a concern. 🤔

[natematykiewicz]

Is there a way to scope all of these to the app/models folder? Right now you’re kind of assuming these are ActiveRecord methods right?

Gems like ActiveModelSerizers define belongs_to and has_many methods for you to use. I assume this code would add symbols for them. Is that a bug or is that a feature? Idk

[vinistock]

I don't think it's a major issue to include the scope in routes. But one easy way to fix it is to remember if we're inside a class or not, which wouldn't happen in the route definitions.

[andyw8]

@natematykiewicz do you want to try that? It would a similar what we do here, but allowing for any class.

[natematykiewicz]

Yup! I'll give it a go.

[natematykiewicz]

How's this look? https://github.com/Shopify/ruby-lsp-rails/pull/333/commits/ffd73e73b6ea7ca9349f253a1b7540d91a070f50

This is my first time using Sorbet. I think I set up that union type right.

I also struggled to figure out how Prism knows to call on_class_node_enter. I couldn't find it in this documentation.

I basically guessed that on_module_node_enter and on_module_node_leave exist based on the fact that Prism::ModuleNode is listed in the documentation.

Without the module ones, the concern's symbols started failing to get indexed.

I wasn't sure if you wanted this class/module check only for scope, or for all of these ActiveRecord methods. I've currently applied it to everything. If you want me to only have that check for scope, I can move that check further down.

[andyw8]

@natematykiewicz thanks!