Skip to content
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

Make requests easier to extend #972

Merged
merged 3 commits into from
Sep 8, 2023
Merged

Make requests easier to extend #972

merged 3 commits into from
Sep 8, 2023

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Sep 5, 2023

Motivation

This is part of #808

Currently, to make a listener-based request extensible, we need to:

  • Update its constructor to initialise an array of external listeners
  • Implement a few interfaces like merge_response! and merge_external_listeners_responses!
  • Remember to call merge_external_listeners_responses! in the executor.rb before accessing the response

It's not easy to remember all of them at the same time. And when it's not implemented correctly, we'd rely on runtime information to figure out what's missing.

So with this change, I want to utilise Sorbet as much as possible, while reducing the APIs that need to be implemented.

Implementation

  • When extending a listener class, we change its parent class from Listener to the new ExtensibleListener class. Sorbet would then require a few more interfaces on its child class than Listener's.
  • To make ExtensibleListener#response merge external responses automatically, I changed Listener's response interface to _response. This will prevent individual listener classes from overriding the response method when declaring its ResponseType.
  • I also changed a few utility scripts (e.g. doc check) as ExtensibleListener is Listener's child class but is abstract.

One shortfall of this approach is that if initialize_external_listener uses ivars to initialise external listeners, super needs to be called after the ivars are initialised. And this could not be type-checked. (Ideally we should prepend a module for such order-dependant requirement, but it's not supported by Sorbet)

Automated Tests

All existing tests should pass

Manual Tests

@st0012 st0012 added the breaking-change Non-backward compatible change label Sep 5, 2023
@st0012 st0012 added this to the 2023-Q3 milestone Sep 5, 2023
@st0012 st0012 force-pushed the introduce-extensible-module branch from 9a211db to 9c7ceb8 Compare September 5, 2023 15:28
@st0012 st0012 self-assigned this Sep 5, 2023
@st0012 st0012 marked this pull request as ready for review September 5, 2023 16:09
@st0012 st0012 requested a review from a team as a code owner September 5, 2023 16:09
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. This will really help ensuring consistency in extensible requests.

What's the breaking change though?

lib/ruby_lsp/listener.rb Show resolved Hide resolved
lib/ruby_lsp/listener.rb Show resolved Hide resolved
@@ -127,8 +127,10 @@ def name
end

def create_code_lens_listener(uri, emitter, message_queue)
raise "uri can't be nil" unless uri
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leverage Sorbet for this instead. We can just add a signature and we shouldn't need this check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried it but maybe I was doing something wrong because it's like wrestling with Sorbet with little benefit due to the dynamic class definition:

  1. We need to add extend T::Sig to CodeLensExpectationsTest instead of the extension test class.

  2. In this case, we need to add override to the signature of create_code_lens_listener. But Sorbet would complain that there's nothing to override about, which I think is also due to dynamic class definition?
    Screenshot 2023-09-07 at 12 20 23

  3. If there's any runtime type checking error, it will be swallowed in RubyLsp::Result instead of being raised

    Screenshot 2023-09-07 at 12 21 31

So eventually I decided that it's not worth the effort here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Yeah, Sorbet doesn't play well with Class.new.

@st0012 st0012 force-pushed the introduce-extensible-module branch from 9c7ceb8 to dd25717 Compare September 7, 2023 11:24
@st0012 st0012 requested a review from vinistock September 7, 2023 13:04
@@ -127,8 +127,10 @@ def name
end

def create_code_lens_listener(uri, emitter, message_queue)
raise "uri can't be nil" unless uri
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. Yeah, Sorbet doesn't play well with Class.new.

This will allow `ExternalListener` to encapsulate response merging logic
under `response`, which means we can always call `response` on the listener
regardless of whether it's an extensible or not.
@st0012 st0012 force-pushed the introduce-extensible-module branch from dd25717 to e3abd29 Compare September 8, 2023 14:28
@st0012 st0012 merged commit 7bd2869 into main Sep 8, 2023
@st0012 st0012 deleted the introduce-extensible-module branch September 8, 2023 15:01
vinistock pushed a commit that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Non-backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants