(GH-177) Refactor message_router as property of Language Client

Previously the message router was passed in as a method parameter however as
almost all methods require the message router, this commit changes the
initializer to pass in the message router at object create time.

Author: glennsarti

lib/puppet-languageserver/language_client.rb

@@ -2,7 +2,10 @@
 
 module PuppetLanguageServer
   class LanguageClient
-    def initialize
+    attr_reader :message_router
+
+    def initialize(message_router)
+      @message_router = message_router
       @client_capabilites = {}
 
       # Internal registry of dynamic registrations and their current state
@@ -20,7 +23,7 @@ def client_capability(*names)
       safe_hash_traverse(@client_capabilites, *names)
     end
 
-    def send_configuration_request(message_router)
+    def send_configuration_request
       params = LSP::ConfigurationParams.new.from_h!('items' => [])
       params.items << LSP::ConfigurationItem.new.from_h!('section' => 'puppet')
 
@@ -45,7 +48,7 @@ def capability_registrations(method)
       @registrations[method].dup
     end
 
-    def register_capability(message_router, method, options = {})
+    def register_capability(method, options = {})
       id = new_request_id
 
       PuppetLanguageServer.log_message(:info, "Attempting to dynamically register the #{method} method with id #{id}")
@@ -67,7 +70,7 @@ def register_capability(message_router, method, options = {})
       true
     end
 
-    def parse_register_capability_response!(message_router, response, original_request)
+    def parse_register_capability_response!(response, original_request)
       raise 'Response is not from client/registerCapability request' unless original_request['method'] == 'client/registerCapability'
 
       unless response.key?('result')
@@ -88,7 +91,7 @@ def parse_register_capability_response!(message_router, response, original_reque
 
         # If we just registered the workspace/didChangeConfiguration method then
         # also trigger a configuration request to get the initial state
-        send_configuration_request(message_router) if reg.method__lsp == 'workspace/didChangeConfiguration'
+        send_configuration_request if reg.method__lsp == 'workspace/didChangeConfiguration'
       end
 
       true

lib/puppet-languageserver/message_router.rb

@@ -34,7 +34,7 @@ class MessageRouter < BaseMessageRouter
     def initialize(options = {})
       super
       @server_options = options.nil? ? {} : options
-      @client = LanguageClient.new
+      @client = LanguageClient.new(self)
     end
 
     def documents
@@ -254,7 +254,7 @@ def receive_notification(method, params)
       when 'initialized'
         PuppetLanguageServer.log_message(:info, 'Client has received initialization')
         if client.client_capability('workspace', 'didChangeConfiguration', 'dynamicRegistration') == true
-          client.register_capability(self, 'workspace/didChangeConfiguration')
+          client.register_capability('workspace/didChangeConfiguration')
         else
           PuppetLanguageServer.log_message(:debug, 'Client does not support didChangeConfiguration dynamic registration. Using push method for configuration change detection.')
         end
@@ -300,7 +300,7 @@ def receive_notification(method, params)
         if params.key?('settings') && params['settings'].nil?
           # This is a notification from a dynamic registration.  Need to send a workspace/configuration
           # request to get the actual configuration
-          client.send_configuration_request(self)
+          client.send_configuration_request
         else
           client.parse_lsp_configuration_settings!(params['settings'])
         end
@@ -314,13 +314,13 @@ def receive_notification(method, params)
     end
 
     def receive_response(response, original_request)
-      unless receive_response_succesful?(response) # rubocop:disable Style/IfUnlessModifier Too long
+      unless receive_response_succesful?(response) # rubocop:disable Style/IfUnlessModifier Line is too long otherwise
         PuppetLanguageServer.log_message(:error, "Response for method '#{original_request['method']}' with id '#{original_request['id']}' failed with #{response['error']}")
       end
       # Error responses still need to be processed so process messages even if it failed
       case original_request['method']
       when 'client/registerCapability'
-        client.parse_register_capability_response!(self, response, original_request)
+        client.parse_register_capability_response!(response, original_request)
       when 'workspace/configuration'
         return unless receive_response_succesful?(response)
         client.parse_lsp_configuration_settings!(response['result'])

spec/languageserver/unit/puppet-languageserver/language_client_spec.rb

@@ -1,8 +1,10 @@
 require 'spec_helper'
 
 describe 'PuppetLanguageServer::LanguageClient' do
+  let(:json_rpc_handler) { MockJSONRPCHandler.new }
+  let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
   let(:subject_options) {}
-  let(:subject) { PuppetLanguageServer::LanguageClient.new }
+  let(:subject) { PuppetLanguageServer::LanguageClient.new(message_router) }
   let(:initialize_params) do
     # Example capabilities from VS Code
     {
@@ -139,8 +141,6 @@
       }
     }
   end
-  let(:json_rpc_handler) { MockJSONRPCHandler.new }
-  let(:message_router) { MockMessageRouter.new.tap { |i| i.json_rpc_handler = json_rpc_handler } }
 
   before(:each) do
     allow(PuppetLanguageServer).to receive(:log_message)
@@ -167,11 +167,11 @@
   describe '#send_configuration_request' do
     it 'should send a client request and return true' do
       expect(json_rpc_handler).to receive(:send_client_request).with('workspace/configuration', Object)
-      expect(subject.send_configuration_request(message_router)).to eq(true)
+      expect(subject.send_configuration_request).to eq(true)
     end
 
     it 'should include the puppet settings' do
-      subject.send_configuration_request(message_router)
+      subject.send_configuration_request
       expect(json_rpc_handler.connection.buffer).to include('{"section":"puppet"}')
     end
   end
@@ -207,13 +207,13 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
     end
@@ -232,13 +232,13 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :pending, :id => request_id}])
       # Mock an error response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete, :id => request_id}])
     end
@@ -258,15 +258,15 @@
       # Should start out not registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => false, :state => :complete}])
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should show registered
       expect(subject.capability_registrations(method_name)).to eq([{:registered => true, :state => :complete, :id => request_id}])
       # Send another registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Should show as in progress
       expect(subject.capability_registrations(method_name)).to eq([
         {:registered => true,  :state => :complete, :id => request_id},
@@ -275,7 +275,7 @@
       # Mock an error response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'error' => { 'code' => -1, 'message' => 'mock message' } }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => req_method_name, 'params' => req_method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
       # Should still show registered
       expect(subject.capability_registrations(method_name)).to eq([
         {:registered => true,  :state => :complete, :id => request_id},
@@ -290,25 +290,25 @@
 
     it 'should send a client request and return true' do
       expect(json_rpc_handler).to receive(:send_client_request).with('client/registerCapability', Object)
-      expect(subject.register_capability(message_router, method_name, method_options)).to eq(true)
+      expect(subject.register_capability(method_name, method_options)).to eq(true)
     end
 
     it 'should include the method to register' do
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       expect(json_rpc_handler.connection.buffer).to include("\"method\":\"#{method_name}\"")
     end
 
     it 'should include the parameters to register' do
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       expect(json_rpc_handler.connection.buffer).to include('"registerOptions":{}')
     end
 
     it 'should log a message if a registration is already in progress' do
       allow(json_rpc_handler).to receive(:send_client_request)
       expect(PuppetLanguageServer).to receive(:log_message).with(:warn, /#{method_name}/)
 
-      subject.register_capability(message_router, method_name, method_options)
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
+      subject.register_capability(method_name, method_options)
     end
 
     it 'should not log a message if a previous registration completed' do
@@ -321,13 +321,13 @@
       end
       expect(PuppetLanguageServer).to_not receive(:log_message).with(:warn, /#{method_name}/)
       # Send as registration request
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
       # Mock a valid response
       response = { 'jsonrpc'=>'2.0', 'id'=> 0, 'result' => nil }
       original_request = { 'jsonrpc'=>'2.0', 'id' => 0, 'method' => method_name, 'params' => method_params }
-      subject.parse_register_capability_response!(message_router, response, original_request)
+      subject.parse_register_capability_response!(response, original_request)
 
-      subject.register_capability(message_router, method_name, method_options)
+      subject.register_capability(method_name, method_options)
     end
   end
 
@@ -342,7 +342,7 @@
       let(:request_params) { {} }
 
       it 'should raise an error if the original request was not a registration' do
-        expect{ subject.parse_register_capability_response!(message_router, response, original_request) }.to raise_error(/client\/registerCapability/)
+        expect{ subject.parse_register_capability_response!(response, original_request) }.to raise_error(/client\/registerCapability/)
       end
     end
 
@@ -355,8 +355,8 @@
       end
 
       it 'should send a configuration request' do
-        expect(subject).to receive(:send_configuration_request).with(message_router)
-        subject.parse_register_capability_response!(message_router, response, original_request)
+        expect(subject).to receive(:send_configuration_request)
+        subject.parse_register_capability_response!(response, original_request)
       end
     end
 
@@ -377,15 +377,15 @@
         it 'should not log the registration' do
           expect(PuppetLanguageServer).to_not receive(:log_message).with(:info, /validMethod/)
 
-          subject.parse_register_capability_response!(message_router, response, original_request)
+          subject.parse_register_capability_response!(response, original_request)
         end
       end
 
       context 'that succeeded' do
         it 'should log the registration' do
           expect(PuppetLanguageServer).to receive(:log_message).with(:info, /validMethod/)
 
-          subject.parse_register_capability_response!(message_router, response, original_request)
+          subject.parse_register_capability_response!(response, original_request)
         end
       end
     end

spec/languageserver/unit/puppet-languageserver/message_router_spec.rb

@@ -858,7 +858,7 @@
         end
 
         it 'should attempt to register workspace/didChangeConfiguration' do
-          expect(subject.client).to receive(:register_capability).with(Object, 'workspace/didChangeConfiguration')
+          expect(subject.client).to receive(:register_capability).with('workspace/didChangeConfiguration')
 
           subject.receive_notification(notification_method, notification_params)
         end
@@ -1037,7 +1037,7 @@
         let(:config_settings) { nil }
 
         it 'should send a configuration request' do
-          expect(subject.client).to receive(:send_configuration_request).with(Object)
+          expect(subject.client).to receive(:send_configuration_request).with(no_args)
 
           subject.receive_notification(notification_method, notification_params)
         end
@@ -1116,7 +1116,7 @@
       let(:request_params) { {} }
 
       it 'should call client.parse_register_capability_response!' do
-        expect(subject.client).to receive(:parse_register_capability_response!).with(Object, response, original_request)
+        expect(subject.client).to receive(:parse_register_capability_response!).with(response, original_request)
 
         subject.receive_response(response, original_request)
       end