Move gem RBI generation to server add-on #2174

andyw8 · 2025-01-29T18:15:08Z

As discussed internally, move this behaviour to the server add-on for consistency with the DSL add-on, and so that we can benefit by using fork.

verify on CodeDB

code-db andyw8/experiemtn % ls -alT sorbet/rbi/gems/[email protected]
-rw-r--r--@ 1 andyw8  staff  62404 Feb  7 12:07:47 2025 sorbet/rbi/gems/[email protected]
code-db andyw8/experiemtn % ls -alT Gemfile.lock                         
-rw-r--r--@ 1 andyw8  staff  21834 Feb  7 12:07:34 2025 Gemfile.lock

(13 seconds)

verify on Core

vinistock

This is missing the plumbing to invoke

spec/tapioca/addon_spec.rb

lib/ruby_lsp/tapioca/server_addon.rb

Gemfile.lock

andyw8 · 2025-02-06T20:37:21Z

spec/tapioca/ruby_lsp/run_gem_rbi_check_spec.rb

          check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
-          check.run
+          check.run { |gem| gem_list.concat(gem) }


(This would be easier to verify if we had Mocha)

andyw8 · 2025-02-07T17:43:39Z

lib/ruby_lsp/tapioca/gem_rbi_check.rb

-      attr_reader :stdout
-      attr_reader :stderr
-      attr_reader :status
+      attr_reader :log


(Renamed from stdout)

The only place we read the logs now is the test, so it may be better to remove this.

The logs in this file are useful no? Why don't we send them to the editor?

andyw8 · 2025-02-07T17:53:04Z

lib/ruby_lsp/tapioca/gem_rbi_check.rb

@@ -6,29 +6,28 @@

 module RubyLsp
  module Tapioca
-    class RunGemRbiCheck
+    class GemRbiCheck


(renamed as per Kaan's suggestion but we're open to other ideas)

andyw8 · 2025-02-07T17:58:29Z

spec/tapioca/ruby_lsp/gem_rbi_check_spec.rb

@@ -57,10 +57,11 @@ class RunGemRbiCheckSpec < SpecWithProject
          @project.require_mock_gem(foo)
          @project.bundle_install!

-          check = ::RubyLsp::Tapioca::RunGemRbiCheck.new(@project.absolute_path)
-          check.run
+          gems_for_tapioca_command = []


If we had Mocha we could write this as something like check.expects(:run).yields(["foo"])

KaanOzkan · 2025-02-07T21:45:32Z

lib/ruby_lsp/tapioca/gem_rbi_check.rb

-      attr_reader :stdout
-      attr_reader :stderr
-      attr_reader :status
+      attr_reader :log


The logs in this file are useful no? Why don't we send them to the editor?

KaanOzkan · 2025-02-07T21:47:35Z

lib/ruby_lsp/tapioca/addon.rb

-          type: Constant::MessageType::WARNING,
-        ) unless gem_rbi_check.stderr.empty?
+        gem_rbi_check = GemRbiCheck.new(T.must(@global_state).workspace_path)
+        gem_rbi_check.run do |gems|


What do you think about this?

Suggested change

gem_rbi_check.run do |gems|

gem_generate.with_modified_gems do |gems|

How about on_gems_modified? I think with is more typically used as a 'wrapper'.

Sure, I think gem_generate better aligns with existing Tapioca commands too, let's make that change as well. Both for local variable and the class.

andyw8 · 2025-02-07T22:40:12Z

lib/ruby_lsp/tapioca/addon.rb

-          gem_rbi_check.stderr,
-          type: Constant::MessageType::WARNING,
-        ) unless gem_rbi_check.stderr.empty?
+        logger_callback = lambda do |message|


Named it to avoid confusion with the built-in Logger.

andyw8 · 2025-02-10T16:26:48Z

Abandoning due to Bundler-related challenges, see https://github.com/Shopify/team-ruby-dx/issues/1372

andyw8 mentioned this pull request Jan 29, 2025

Fix chdir issue for LSP #2171

Merged

andyw8 added the enhancement New feature or request label Jan 30, 2025

andyw8 force-pushed the andyw8/move-gem-rbi-gen-to-server branch 2 times, most recently from ce44c9a to ea38d97 Compare January 31, 2025 21:23

andyw8 changed the title ~~WIP: Move gem RBI gen to server~~ Move gem RBI generation to server add-on Jan 31, 2025

andyw8 marked this pull request as ready for review February 3, 2025 18:18

andyw8 requested a review from a team as a code owner February 3, 2025 18:18

vinistock reviewed Feb 3, 2025

View reviewed changes

spec/tapioca/addon_spec.rb Outdated Show resolved Hide resolved

lib/ruby_lsp/tapioca/server_addon.rb Outdated Show resolved Hide resolved

KaanOzkan reviewed Feb 6, 2025

View reviewed changes

lib/ruby_lsp/tapioca/server_addon.rb Outdated Show resolved Hide resolved

lib/ruby_lsp/tapioca/server_addon.rb Show resolved Hide resolved

andyw8 marked this pull request as draft February 6, 2025 16:32

andyw8 commented Feb 6, 2025

View reviewed changes

Gemfile.lock Outdated Show resolved Hide resolved

wip

a2538f9

andyw8 force-pushed the andyw8/move-gem-rbi-gen-to-server branch from a00daf4 to a2538f9 Compare February 6, 2025 18:25

andyw8 added 8 commits February 6, 2025 13:28

No need to handle removals here

ebdc947

Remove @stderr

565ccbb

Use guard

6197035

Inline method

4222108

Pass a block

792e7ae

Restore sig

5cf7c49

Remove stale comment

5db674f

Update some tests

1dc33c6

andyw8 force-pushed the andyw8/move-gem-rbi-gen-to-server branch from 95e7e13 to 0773913 Compare February 6, 2025 20:31

andyw8 commented Feb 6, 2025

View reviewed changes

Update tests

c4fdf4f

andyw8 force-pushed the andyw8/move-gem-rbi-gen-to-server branch from 0773913 to c4fdf4f Compare February 6, 2025 20:40

andyw8 added 5 commits February 6, 2025 15:47

Rename class

24c2664

Try fix flaky test

f24be17

Removed unused

a40d788

Make callback mandatory

ca9d51e

Use assert_empty

db7baa7

andyw8 added 3 commits February 7, 2025 11:56

Naming

5d9a880

Rename @stdout

0eb1329

Cleanup

df2f082

andyw8 marked this pull request as ready for review February 7, 2025 17:41

andyw8 commented Feb 7, 2025

View reviewed changes

Logging

3b175e3

KaanOzkan reviewed Feb 7, 2025

View reviewed changes

Add logging support

af64163

andyw8 commented Feb 7, 2025

View reviewed changes

andyw8 closed this Feb 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move gem RBI generation to server add-on #2174

Move gem RBI generation to server add-on #2174

andyw8 commented Jan 29, 2025 •

edited

Loading

vinistock left a comment

andyw8 Feb 6, 2025

andyw8 Feb 7, 2025 •

edited

Loading

KaanOzkan Feb 7, 2025

andyw8 Feb 7, 2025 •

edited

Loading

andyw8 Feb 7, 2025

KaanOzkan Feb 7, 2025

KaanOzkan Feb 7, 2025

andyw8 Feb 7, 2025

KaanOzkan Feb 10, 2025

andyw8 Feb 7, 2025

andyw8 commented Feb 10, 2025

	gem_rbi_check.run do \|gems\|
	gem_generate.with_modified_gems do \|gems\|

Move gem RBI generation to server add-on #2174

Move gem RBI generation to server add-on #2174

Conversation

andyw8 commented Jan 29, 2025 • edited Loading

vinistock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 Feb 7, 2025 • edited Loading

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

andyw8 commented Feb 10, 2025

andyw8 commented Jan 29, 2025 •

edited

Loading

andyw8 Feb 7, 2025 •

edited

Loading

andyw8 Feb 7, 2025 •

edited

Loading