-
Notifications
You must be signed in to change notification settings - Fork 136
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
[Tapioca Add-on] Trigger DSL generation upon file changes #2031
[Tapioca Add-on] Trigger DSL generation upon file changes #2031
Conversation
9b590a0
to
eef6cb2
Compare
lib/ruby_lsp/tapioca/addon.rb
Outdated
constants = changes.filter_map do |change| | ||
path = change[:uri].gsub("file://", "") | ||
|
||
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Class, RubyIndexer::Entry::Module) |
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.
Using grep
, getting subclasses and then filtering them out feels weird to me. Wdyt of an API like this where you have a rest arg and use ==
instead? Or making the second argument optional and returning all entries by default?
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.
You can just use the parent class to filter both at the same time.
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Class, RubyIndexer::Entry::Module) | |
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Namespace) |
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 can't only use the parent class because it includes singleton classes. So I'm trying to avoid this filtering with a different API in Ruby LSP main...andyw8/tapioca-lsp#diff-8243d0258619c7955b4277251f121d4b6788da80bcae39fe5130f27019d143bbR64
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 think for our use case being able to omit the types and do our own filtering in tapioca makes sense. Will you accept that change in Ruby LSP?
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 mean, we created that API for the Tapioca add-on, so we can change it to whatever makes sense.
lib/ruby_lsp/tapioca/addon.rb
Outdated
constants = changes.filter_map do |change| | ||
path = change[:uri].gsub("file://", "") | ||
|
||
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Class, RubyIndexer::Entry::Module) |
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.
You can just use the parent class to filter both at the same time.
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Class, RubyIndexer::Entry::Module) | |
entries = T.must(@index).entries_for(path, RubyIndexer::Entry::Namespace) |
dfa196d
to
479d175
Compare
479d175
to
964ca6b
Compare
b6aac6a
into
tapioca-addon-feature-branch
We need to also update the # We need to change this to >= 0.3.18
addon = T.cast(::RubyLsp::Addon.get("Ruby LSP Rails", ">= 0.3.17", "< 0.4"), ::RubyLsp::Rails::Addon) |
Motivation
Merge part of tapioca add-on logic
Implementation
workspace_did_change_watched_files
method that uses the Ruby LSP index to find potentially modified constants in a file and sends them to the Rails appTests
We'll add tests when we can test rails client