From 8e1bc9ea4e4d852c4ed0eabd65e587a1908a6edd Mon Sep 17 00:00:00 2001
From: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
Date: Wed, 18 Oct 2023 22:49:04 +0300
Subject: [PATCH] Refactor RuboCop diagnostics for better encapsulation

The code description addition for RuboCop diagnostics has added a cop lookup to the diagnostics runner, but I feel like that was an abstraction added at the wrong layer.

What we really want to cache is the registry as a hash lookup but we want to do that as late as possible to ensure that we have loaded the full registry. For that reason, I moved the actual cop lookup by cop name to the RuboCop runner class as a class method, which uses a memoized registry hash under the hood.

This way, we can keep the code of the RuboCop diagnostics class clean and with less conditional code.

I've also taken the liberty to extract other calculation logic from inside the `to_lsp_diagnostic` method to separate methods, to make the intent more clear.
---
 .../requests/support/rubocop_diagnostic.rb    | 34 ++++++++++---------
 .../requests/support/rubocop_runner.rb        | 19 +++++++++++
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/lib/ruby_lsp/requests/support/rubocop_diagnostic.rb b/lib/ruby_lsp/requests/support/rubocop_diagnostic.rb
index 46ccd0352..268fdb446 100644
--- a/lib/ruby_lsp/requests/support/rubocop_diagnostic.rb
+++ b/lib/ruby_lsp/requests/support/rubocop_diagnostic.rb
@@ -19,12 +19,6 @@ class RuboCopDiagnostic
           T::Hash[Symbol, Integer],
         )
 
-        # Cache cops to attach URLs to diagnostics. Only built-in cops for now.
-        COP_TO_DOC_URL = T.let(
-          RuboCop::Cop::Registry.global.to_h,
-          T::Hash[String, [T.class_of(RuboCop::Cop::Base)]],
-        )
-
         sig { params(offense: RuboCop::Cop::Offense, uri: URI::Generic).void }
         def initialize(offense, uri)
           @offense = offense
@@ -53,16 +47,6 @@ def to_lsp_code_action
 
         sig { returns(Interface::Diagnostic) }
         def to_lsp_diagnostic
-          severity = RUBOCOP_TO_LSP_SEVERITY[@offense.severity.name]
-          message  = @offense.message
-
-          message += "\n\nThis offense is not auto-correctable.\n" unless @offense.correctable?
-
-          cop = COP_TO_DOC_URL[@offense.cop_name]&.first
-          if cop&.documentation_url
-            code_description = { href: cop.documentation_url }
-          end
-
           Interface::Diagnostic.new(
             message: message,
             source: "RuboCop",
@@ -88,6 +72,24 @@ def to_lsp_diagnostic
 
         private
 
+        sig { returns(String) }
+        def message
+          message  = @offense.message
+          message += "\n\nThis offense is not auto-correctable.\n" unless @offense.correctable?
+          message
+        end
+
+        sig { returns(T.nilable(Integer)) }
+        def severity
+          RUBOCOP_TO_LSP_SEVERITY[@offense.severity.name]
+        end
+
+        sig { returns(T.nilable(Interface::CodeDescription)) }
+        def code_description
+          doc_url = RuboCopRunner.find_cop_by_name(@offense.cop_name)&.documentation_url
+          Interface::CodeDescription.new(href: doc_url) if doc_url
+        end
+
         sig { returns(T::Array[Interface::TextEdit]) }
         def offense_replacements
           @offense.corrector.as_replacements.map do |range, replacement|
diff --git a/lib/ruby_lsp/requests/support/rubocop_runner.rb b/lib/ruby_lsp/requests/support/rubocop_runner.rb
index 4e9d2035f..65eb3039d 100644
--- a/lib/ruby_lsp/requests/support/rubocop_runner.rb
+++ b/lib/ruby_lsp/requests/support/rubocop_runner.rb
@@ -101,6 +101,25 @@ def formatted_source
           @options[:stdin]
         end
 
+        class << self
+          extend T::Sig
+
+          sig { params(cop_name: String).returns(T.nilable(T.class_of(RuboCop::Cop::Base))) }
+          def find_cop_by_name(cop_name)
+            cop_registry[cop_name]&.first
+          end
+
+          private
+
+          sig { returns(T::Hash[String, [T.class_of(RuboCop::Cop::Base)]]) }
+          def cop_registry
+            @cop_registry ||= T.let(
+              RuboCop::Cop::Registry.global.to_h,
+              T.nilable(T::Hash[String, [T.class_of(RuboCop::Cop::Base)]]),
+            )
+          end
+        end
+
         private
 
         sig { params(_file: String, offenses: T::Array[RuboCop::Cop::Offense]).void }