-
Notifications
You must be signed in to change notification settings - Fork 21
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 reloading for schema changes #282
Conversation
@@ -215,10 +215,6 @@ def hover_on_source(source, position) | |||
assert_nil(response.error) | |||
response.response | |||
end | |||
|
|||
def dummy_root |
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.
Move into test_helper so that we can share.
sig { params(changes: T::Array[{ uri: String, type: Integer }]).void } | ||
def workspace_did_change_watched_files(changes) | ||
if changes.any? do |change| | ||
change[:uri].end_with?("db/schema.rb") || change[:uri].end_with?("structure.sql") |
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.
Normally we would only have to support db/structure.sql
but in Shopify/shopify
we have additional structure files.
def trigger_reload | ||
warn("Reloading Rails application...") | ||
send_notification("reload") | ||
warn("Reloading Rails application complete") |
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.
@vinistock I realised it might be better to remove the second warn
here, since that call is asynchronous?
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.
That makes sense. We can either remove it or turn the notification into a request and just return an OK
from the server.
IMO, I'd go with the former. No need for extra complexity.
ee951f7
to
7ceefbb
Compare
warn("Reloading Rails application") | ||
send_notification("reload") | ||
rescue IncompleteMessageError | ||
warn("Ruby LSP Rails failed to get model information: #{@stderr.read}") |
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.
Will correct this message.
7ceefbb
to
5f93af0
Compare
This change goes along with Shopify/ruby-lsp#1464 in ruby-lsp.
Partly addresses #257
We register for changes to the schema files, and trigger a reload so as to get the latest column information. (Using the same mechanism that
reload!
in the Rails console uses).