Fix NullClient#send_notification
#426
Conversation
| alias_method :send_notification, :send_message | ||
| # Notifications are messages that do not expect a response | ||
| sig { params(request: String, params: T.nilable(T::Hash[Symbol, T.untyped])).void } | ||
| def send_notification(request, params = nil) = send_message(request, params) |
There was a problem hiding this comment.
I chose this over the alternatives, so subclasses don't have to remember to add their own alias/override.
It's unlikely that somebody would not want it to just mean the same thing as send_message, but if they did, they still have the freedom to override it different, if they would like.
| end | ||
|
|
||
| test "#send_notification is a no-op" do | ||
| assert_nothing_raised { @client.send(:send_notification, "request", nil) } |
There was a problem hiding this comment.
Prior to the fix, this failed with:
Error:
RubyLsp::Rails::NullClientTest#test_#send_notification_is_a_no-op:
NoMethodError: undefined method `write' for nil
lib/ruby_lsp/ruby_lsp_rails/runner_client.rb:180:in `send_message'
test/ruby_lsp_rails/runner_client_test.rb:111:in `block (2 levels) in <class:NullClientTest>'
/Users/alex/.gem/ruby/3.3.0/gems/activesupport-7.1.3.3/lib/active_support/testing/assertions.rb:49:in `assert_nothing_raised'
test/ruby_lsp_rails/runner_client_test.rb:111:in `block in <class:NullClientTest>'
|
For future safety, would it be better to have |
|
I've removed myself as reviewer for now, you can re-add me once the discussions are resolved. |
amomchilov
force-pushed
the
Alex/fix-NullClient-crasher
branch
from
aa5b6ba to
180ca75
Compare
|
@andyw8 I played around with it, but I don't think the result is particularly better. The two are just fairly overlapping. e3df69f Perhaps there's room to extract a more minimal |
Fixes #425