Integrate builder pattern to refactor hover responses #1317

aryan-soni · 2024-01-21T23:22:59Z

Motivation

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

Eventually, we want to facilitate deterministic categorization for hover responses. As part of this, we should first encapsulate our responses into a HoverResponseBuilder class, which will then handle the ordering of content in the future.

Implementation

The previous approach for hover responses had some suboptimal aspects:

Within Requests::Hover, the perform method was responsible for constructing the response in a convoluted way.
The listener was responsible for instantiating Hover objects.
We were always passing in the range when constructing a Hover response, but as per the LSP spec, we weren't using this range in a meaningful fashion.

The new approach ensures:

We abstract away response construction to a builder. When we add deterministic categorization for hover responses, this will ensure that we have a concrete separation of concerns.
We now only construct a Hover object at one time, within Requests::Hover instead of the listener. This optimizes runtime as it ensures we limit the construction of objects.
We can disregard the range.
There is a more general ResponseBuilder class that we can use to refactor responses for other areas of the LSP as well.

Automated Tests

In hover_expectations_test.rb, we don't want to modify the expectations themselves (as the contract persists), but we can change the initialization work done to facilitate the tests such that it now reflects our changes.

We can also updated our broader expectations to remove range, since we are now phasing this out.

Manual Tests

We're not adding/removing any functionality, but are simply laying the foundation for improvements later in the future. As a consequence, testing this involves ensuring that our hover functionality is unchanged.

As we can see, the hover functionality persists:

aryan-soni · 2024-01-21T23:24:58Z

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.


Thought it would be helpful to add a note here since the code below will no longer be accurate for Hover

Yeah, we'll need to update all of the docs. It might be worth waiting until the refactors are complete and then we update them once.

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

st0012 · 2024-01-22T20:51:35Z

lib/ruby_lsp/requests/hover.rb

@@ -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]])


I think we shouldn't expect the addons' implementation to inherit from Listener either. They'd be plain old Ruby object that takes a dispatcher as Listeners::Hover.

Eventually, the Listener class will be removed after all these restructuring, or at least be much simpler than it currently is.

Looks like we can remove this line since we don't use it elsewhere regardless.

Agreed with Stan's comment about the future structure of listeners, but indeed we can just remove this line since it's no longer used.

st0012 · 2024-01-22T20:55:45Z

lib/ruby_lsp/response_builder.rb

+module RubyLsp
+  class OperationNotPermitted < StandardError; end
+
+  class ResponseBuilder


Can we use the simple Response abstract class in response.rb for now? We won't release it as it is of course. But keeping it simple until other requests are restructured will make it easier to spot the commonality.

Yeah, I agree we should unify the abstract classes.

I'll make the change, but to your point, I definitely think we should narrow in to a common builder class before we proceed with the release.

I decided to adopt the builder concept in #1325 (with a slightly different form of structure).

But I still think we should not add more common APIs before we've converted most requests. For example, we probably don't want addons to have the pop API for semantic highlighting.

I think it would be best to finalize on an approach for the builder in this PR, get this merged, then you can rebase in your PR. I can factor in your approach and modify mine accordingly.

Thoughts? Just thinking of the best way to minimize conflicts.

I'm obviously biased toward my PR 😛 But consider it already adopted #1317 (comment), it may actually be easier if you rebase yours on mine.

Makes sense! 😄

test/expectations/hover/documented_class.exp.json

vinistock · 2024-01-25T18:17:30Z

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.


Yeah, we'll need to update all of the docs. It might be worth waiting until the refactors are complete and then we update them once.

vinistock · 2024-01-25T18:18:08Z

lib/ruby_lsp/addon.rb

@@ -1,6 +1,8 @@
 # typed: strict
 # frozen_string_literal: true

+require_relative "response_builder"


This will be necessary for most of the LSP, so I think we can move this require to ruby_lsp/internal where we require most parts.

vinistock · 2024-01-25T18:18:24Z

lib/ruby_lsp/listeners/hover.rb

@@ -1,13 +1,13 @@
 # typed: strict
 # frozen_string_literal: true

+require "ruby_lsp/response_builder"


If we move to internal, then we don't need this one.

vinistock · 2024-01-25T18:18:56Z

lib/ruby_lsp/requests/hover.rb

@@ -2,6 +2,7 @@
 # frozen_string_literal: true

 require "ruby_lsp/listeners/hover"
+require "ruby_lsp/response_builder"


This can be removed too.

vinistock · 2024-01-25T18:19:44Z

lib/ruby_lsp/requests/hover.rb

@@ -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]])


Agreed with Stan's comment about the future structure of listeners, but indeed we can just remove this line since it's no longer used.

vinistock · 2024-01-25T18:22:05Z

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)


Can you please double-check if we use this method anywhere else? I kind of expected stuff to break when we changed the type here.

If I'm not mistaken we use it for documentation somewhere. Since it now returns a String instead of an Interface::MarkupContent, we need to do the same thing we did in hover and instantiate it.

Interface::MarkupContent.new( kind: "markdown", value: markdown_from_index_entries(...), )

Great point — see #1328, where I've modified signature_help.rb and completion.rb as you've described.

aryan-soni · 2024-01-28T23:07:33Z

Closing this PR in favour of #1328

aryan-soni commented Jan 21, 2024

View reviewed changes

Integrate builder pattern to refactor hover responses

3526517

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

aryan-soni force-pushed the as/response-refactor branch from fa1372f to 3526517 Compare January 21, 2024 23:27

aryan-soni marked this pull request as ready for review January 21, 2024 23:29

aryan-soni requested a review from a team as a code owner January 21, 2024 23:29

aryan-soni requested review from andyw8, Morriar, vinistock and st0012 January 21, 2024 23:29

st0012 reviewed Jan 22, 2024

View reviewed changes

vinistock reviewed Jan 25, 2024

View reviewed changes

aryan-soni closed this Jan 28, 2024

aryan-soni deleted the as/response-refactor branch January 29, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate builder pattern to refactor hover responses #1317

Integrate builder pattern to refactor hover responses #1317

aryan-soni commented Jan 21, 2024 •

edited

Loading

aryan-soni Jan 21, 2024

vinistock Jan 25, 2024

st0012 Jan 22, 2024

aryan-soni Jan 25, 2024

vinistock Jan 25, 2024

st0012 Jan 22, 2024

aryan-soni Jan 25, 2024

st0012 Jan 25, 2024

aryan-soni Jan 25, 2024

st0012 Jan 25, 2024

aryan-soni Jan 25, 2024

vinistock Jan 25, 2024

vinistock Jan 25, 2024

vinistock Jan 25, 2024

vinistock Jan 25, 2024

vinistock Jan 25, 2024

vinistock Jan 25, 2024

aryan-soni Jan 28, 2024

aryan-soni commented Jan 28, 2024

Integrate builder pattern to refactor hover responses #1317

Integrate builder pattern to refactor hover responses #1317

Conversation

aryan-soni commented Jan 21, 2024 • edited Loading

Motivation

Implementation

Automated Tests

Manual Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryan-soni commented Jan 28, 2024

aryan-soni commented Jan 21, 2024 •

edited

Loading