Integrate builder pattern to refactor hover responses

Hover response construction and manipulation can be delegated to a builder class to promote extensibility and encapsulation.

ADDONS.md

@@ -129,6 +129,7 @@ module RubyLsp
     end
 
     # All listeners have to inherit from ::RubyLsp::Listener
+    # NOTE: Listeners are currently being refactored, but this implementation embodies the current approach for most listeners.
     class Hover < ::RubyLsp::Listener
       extend T::Sig
       extend T::Generic

lib/ruby_lsp/addon.rb

@@ -1,6 +1,8 @@
 # typed: strict
 # frozen_string_literal: true
 
+require_relative "response_builder"
+
 module RubyLsp
   # To register an addon, inherit from this class and implement both `name` and `activate`
   #
@@ -118,12 +120,13 @@ def create_code_lens_listener(uri, dispatcher); end
     # Creates a new Hover listener. This method is invoked on every Hover request
     sig do
       overridable.params(
+        response_builder: RubyLsp::HoverResponseBuilder,
         nesting: T::Array[String],
         index: RubyIndexer::Index,
         dispatcher: Prism::Dispatcher,
       ).returns(T.nilable(Listener[T.nilable(Interface::Hover)]))
     end
-    def create_hover_listener(nesting, index, dispatcher); end
+    def create_hover_listener(response_builder, nesting, index, dispatcher); end
 
     # Creates a new DocumentSymbol listener. This method is invoked on every DocumentSymbol request
     sig do

lib/ruby_lsp/listeners/hover.rb

@@ -1,13 +1,13 @@
 # typed: strict
 # frozen_string_literal: true
 
+require "ruby_lsp/response_builder"
+
 module RubyLsp
   module Listeners
-    class Hover < Listener
+    class Hover
       extend T::Sig
-      extend T::Generic
-
-      ResponseType = type_member { { fixed: T.nilable(Interface::Hover) } }
+      include Requests::Support::Common
 
       ALLOWED_TARGETS = T.let(
         [
@@ -19,26 +19,23 @@ class Hover < Listener
         T::Array[T.class_of(Prism::Node)],
       )
 
-      sig { override.returns(ResponseType) }
-      attr_reader :_response
-
       sig do
         params(
+          response_builder: RubyLsp::HoverResponseBuilder,
           uri: URI::Generic,
           nesting: T::Array[String],
           index: RubyIndexer::Index,
           dispatcher: Prism::Dispatcher,
           typechecker_enabled: T::Boolean,
         ).void
       end
-      def initialize(uri, nesting, index, dispatcher, typechecker_enabled)
+      def initialize(response_builder, uri, nesting, index, dispatcher, typechecker_enabled) # rubocop:disable Metrics/ParameterLists
+        @response_builder = response_builder
         @path = T.let(uri.to_standardized_path, T.nilable(String))
         @nesting = nesting
         @index = index
         @typechecker_enabled = typechecker_enabled
-        @_response = T.let(nil, ResponseType)
 
-        super(dispatcher)
         dispatcher.register(
           self,
           :on_constant_read_node_enter,
@@ -86,12 +83,7 @@ def on_call_node_enter(node)
         target_method = @index.resolve_method(message, @nesting.join("::"))
         return unless target_method
 
-        location = target_method.location
-
-        @_response = Interface::Hover.new(
-          range: range_from_location(location),
-          contents: markdown_from_index_entries(message, target_method),
-        )
+        @response_builder << markdown_from_index_entries(message, target_method)
       end
 
       private
@@ -106,10 +98,7 @@ def generate_hover(name, location)
         first_entry = T.must(entries.first)
         return if first_entry.visibility == :private && first_entry.name != "#{@nesting.join("::")}::#{name}"
 
-        @_response = Interface::Hover.new(
-          range: range_from_location(location),
-          contents: markdown_from_index_entries(name, entries),
-        )
+        @response_builder << markdown_from_index_entries(name, entries)
       end
 
       sig { params(node: Prism::CallNode).void }
@@ -138,13 +127,7 @@ def generate_gem_hover(node)
           #{info}
         MARKDOWN
 
-        @_response = Interface::Hover.new(
-          range: range_from_location(node.location),
-          contents: Interface::MarkupContent.new(
-            kind: Constant::MarkupKind::MARKDOWN,
-            value: markdown,
-          ),
-        )
+        @response_builder << markdown
       rescue Gem::MissingSpecError
         # Do nothing if the spec cannot be found
       end

lib/ruby_lsp/requests/hover.rb

@@ -2,6 +2,7 @@
 # frozen_string_literal: true
 
 require "ruby_lsp/listeners/hover"
+require "ruby_lsp/response_builder"
 
 module RubyLsp
   module Requests
@@ -52,19 +53,16 @@ def initialize(document, index, position, dispatcher, typechecker_enabled)
           target = parent
         end
 
-        @listeners = T.let([], T::Array[Listener[ResponseType]])
+        @listeners = T.let([], T::Array[Listener[HoverResponseBuilder]])
 
         # Don't need to instantiate any listeners if there's no target
         return unless target
 
         uri = document.uri
-        @listeners = T.let(
-          [Listeners::Hover.new(uri, nesting, index, dispatcher, typechecker_enabled)],
-          T::Array[Listener[ResponseType]],
-        )
+        @response_builder = T.let(HoverResponseBuilder.new, HoverResponseBuilder)
+        Listeners::Hover.new(@response_builder, uri, nesting, index, dispatcher, typechecker_enabled)
         Addon.addons.each do |addon|
-          addon_listener = addon.create_hover_listener(nesting, index, dispatcher)
-          @listeners << addon_listener if addon_listener
+          addon.create_hover_listener(@response_builder, nesting, index, dispatcher)
         end
 
         @target = T.let(target, Prism::Node)
@@ -74,18 +72,15 @@ def initialize(document, index, position, dispatcher, typechecker_enabled)
       sig { override.returns(ResponseType) }
       def perform
         @dispatcher.dispatch_once(@target)
-        responses = @listeners.map(&:response).compact
-
-        first_response, *other_responses = responses
 
-        return unless first_response
+        return if @response_builder.empty?
 
-        # TODO: other_responses should never be nil. Check Sorbet
-        T.must(other_responses).each do |other_response|
-          first_response.contents.value << "\n\n" << other_response.contents.value
-        end
-
-        first_response
+        Interface::Hover.new(
+          contents: Interface::MarkupContent.new(
+            kind: "markdown",
+            value: @response_builder.build_concatenated_response,
+          ),
+        )
       end
     end
   end

lib/ruby_lsp/requests/support/common.rb

@@ -86,7 +86,7 @@ def self_receiver?(node)
           params(
             title: String,
             entries: T.any(T::Array[RubyIndexer::Entry], RubyIndexer::Entry),
-          ).returns(Interface::MarkupContent)
+          ).returns(String)
         end
         def markdown_from_index_entries(title, entries)
           markdown_title = "```ruby\n#{title}\n```"
@@ -108,16 +108,13 @@ def markdown_from_index_entries(title, entries)
             content << "\n\n#{entry.comments.join("\n")}" unless entry.comments.empty?
           end
 
-          Interface::MarkupContent.new(
-            kind: "markdown",
-            value: <<~MARKDOWN.chomp,
-              #{markdown_title}
+          <<~MARKDOWN.chomp
+            #{markdown_title}
 
-              **Definitions**: #{definitions.join(" | ")}
+            **Definitions**: #{definitions.join(" | ")}
 
-              #{content}
-            MARKDOWN
-          )
+            #{content}
+          MARKDOWN
         end
       end
     end

lib/ruby_lsp/response_builder.rb

@@ -0,0 +1,50 @@
+# typed: strict
+# frozen_string_literal: true
+
+module RubyLsp
+  class OperationNotPermitted < StandardError; end
+
+  class ResponseBuilder
+    extend T::Sig
+    extend T::Generic
+
+    Elem = type_member { { upper: Object } }
+
+    sig { void }
+    def initialize
+      @response = T.let([], T::Array[Elem])
+    end
+
+    sig { params(elem: Elem).void }
+    def <<(elem)
+      @response << elem
+    end
+
+    sig { returns(T.nilable(Elem)) }
+    def pop
+      @response.pop
+    end
+
+    sig { returns(T::Boolean) }
+    def empty?
+      @response.empty?
+    end
+  end
+
+  class HoverResponseBuilder < ResponseBuilder
+    extend T::Sig
+    extend T::Generic
+
+    Elem = type_member { { fixed: String } }
+
+    sig { returns(String) }
+    def build_concatenated_response
+      @response.join("\n\n")
+    end
+
+    sig { override.returns(T.nilable(Elem)) }
+    def pop
+      raise OperationNotPermitted, "Cannot pop from a HoverResponseBuilder"
+    end
+  end
+end

test/expectations/hover/documented_class.exp.json

@@ -9,16 +9,6 @@
         "contents": {
             "kind": "markdown",
             "value": "```ruby\nBar\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-3,4) | [fake.rb](file:///fake.rb#L6,1-7,4)\n\n\n\nThis is the documentation for Bar\n\nThis is more documentation for Bar"
-        },
-        "range": {
-            "start": {
-                "line": 1,
-                "character": 6
-            },
-            "end": {
-                "line": 1,
-                "character": 9
-            }
         }
     }
 }

test/expectations/hover/documented_constant.exp.json

@@ -9,16 +9,6 @@
         "contents": {
             "kind": "markdown",
             "value": "```ruby\nBAZ\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-2,10)\n\n\n\nThis is the documentation for Baz"
-        },
-        "range": {
-            "start": {
-                "line": 1,
-                "character": 0
-            },
-            "end": {
-                "line": 1,
-                "character": 3
-            }
         }
     }
 }

test/expectations/hover/documented_module.exp.json

@@ -9,16 +9,6 @@
         "contents": {
             "kind": "markdown",
             "value": "```ruby\nFoo\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-3,4)\n\n\n\nThis is the documentation for Foo"
-        },
-        "range": {
-            "start": {
-                "line": 1,
-                "character": 7
-            },
-            "end": {
-                "line": 1,
-                "character": 10
-            }
         }
     }
 }

test/expectations/hover/documented_namespaced_class.exp.json

@@ -9,16 +9,6 @@
         "contents": {
             "kind": "markdown",
             "value": "```ruby\nFoo::Bar\n```\n\n**Definitions**: [fake.rb](file:///fake.rb#L2,1-3,4) | [fake.rb](file:///fake.rb#L6,1-7,4) | [fake.rb](file:///fake.rb#L11,3-12,6)\n\n\n\nThis is the documentation for Foo::Bar\n\nThis is more documentation for Foo::Bar\n\nThis is even more documentation for Foo::Bar"
-        },
-        "range": {
-            "start": {
-                "line": 1,
-                "character": 6
-            },
-            "end": {
-                "line": 1,
-                "character": 14
-            }
         }
     }
 }

test/requests/hover_expectations_test.rb

@@ -261,26 +261,19 @@ def name
         "HoverAddon"
       end
 
-      def create_hover_listener(nesting, index, dispatcher)
-        klass = Class.new(RubyLsp::Listener) do
-          attr_reader :_response
-
-          def initialize(dispatcher)
-            super
+      def create_hover_listener(response_builder, nesting, index, dispatcher)
+        klass = Class.new do
+          def initialize(response_builder, dispatcher)
+            @response_builder = response_builder
             dispatcher.register(self, :on_constant_read_node_enter)
           end
 
           def on_constant_read_node_enter(node)
-            T.bind(self, RubyLsp::Listener[T.untyped])
-            contents = RubyLsp::Interface::MarkupContent.new(
-              kind: "markdown",
-              value: "Hello from middleware: #{node.slice}",
-            )
-            @_response = RubyLsp::Interface::Hover.new(range: range_from_location(node.location), contents: contents)
+            @response_builder << "Hello from middleware: #{node.slice}"
           end
         end
 
-        klass.new(dispatcher)
+        klass.new(response_builder, dispatcher)
       end
     end
   end