Display line diffs in dsl --verify error messages

When `dsl --verify` detects out of date RBI files,
the error message lists which files were added,
removed, or changed. We could make it easier to
understand what has changed by including the line
diff output in the error message as well

This commit grabs the line diff output during RBI
verification and includes it in the error message
when the diff is 250 lines or less

Author: dejmedus

README.md

@@ -507,6 +507,8 @@ Options:
   -w,        [--workers=N]                                                                                  # Number of parallel workers to use when generating RBIs (default: auto)
              [--rbi-max-line-length=N]                                                                      # Set the max line length of generated RBIs. Signatures longer than the max line length will be wrapped
                                                                                                             # Default: 120
+             [--max-diff-lines=N]                                                                           # Max number of diff lines to include in the `dsl --verify` output
+                                                                                                            # Default: 250
   -e,        [--environment=ENVIRONMENT]                                                                    # The Rack/Rails environment to use when generating RBIs
                                                                                                             # Default: development
   -l,        [--list-compilers], [--no-list-compilers], [--skip-list-compilers]                             # List all loaded compilers
@@ -1002,6 +1004,7 @@ dsl:
   quiet: false
   workers: 1
   rbi_max_line_length: 120
+  max_diff_lines: 250
   environment: development
   list_compilers: false
   app_root: "."

lib/tapioca.rb

@@ -32,6 +32,7 @@ class Error < StandardError; end
 
   DEFAULT_RBI_MAX_LINE_LENGTH = 120
   DEFAULT_ENVIRONMENT = "development"
+  DEFAULT_MAX_DIFF_LINES = 250
 
   CENTRAL_REPO_ROOT_URI = "https://raw.githubusercontent.com/Shopify/rbi-central/main"
   CENTRAL_REPO_INDEX_PATH = "index.json"

lib/tapioca/cli.rb

@@ -120,6 +120,10 @@ def todo
       type: :numeric,
       desc: "Set the max line length of generated RBIs. Signatures longer than the max line length will be wrapped",
       default: DEFAULT_RBI_MAX_LINE_LENGTH
+    option :max_diff_lines,
+      type: :numeric,
+      desc: "Max number of diff lines to include in the `dsl --verify` output",
+      default: DEFAULT_MAX_DIFF_LINES
     option :environment,
       aliases: ["-e"],
       type: :string,
@@ -159,6 +163,10 @@ def dsl(*constant_or_paths)
       # Assume anything starting with a capital letter or colon is a class, otherwise a path
       constants, paths = constant_or_paths.partition { |c| c =~ /\A[A-Z:]/ }
 
+      unless options[:max_diff_lines].positive?
+        raise MalformattedArgumentError, "Option '--max-diff-lines' must be a positive number"
+      end
+
       command_args = {
         requested_constants: constants,
         requested_paths: paths.map { |p| Pathname.new(p) },
@@ -176,6 +184,7 @@ def dsl(*constant_or_paths)
         halt_upon_load_error: options[:halt_upon_load_error],
         compiler_options: options[:compiler_options],
         lsp_addon: self.class.addon_mode,
+        max_diff_lines: options[:max_diff_lines],
       }
 
       command = if options[:verify]

lib/tapioca/commands/abstract_dsl.rb

@@ -5,6 +5,7 @@ module Tapioca
   module Commands
     # @abstract
     class AbstractDsl < CommandWithoutTracker
+      include FileHelper
       include SorbetHelper
       include RBIFilesHelper
 
@@ -26,7 +27,8 @@ class AbstractDsl < CommandWithoutTracker
       #|   ?app_root: String,
       #|   ?halt_upon_load_error: bool,
       #|   ?compiler_options: Hash[String, untyped],
-      #|   ?lsp_addon: bool
+      #|   ?lsp_addon: bool,
+      #|   ?max_diff_lines: Integer
       #| ) -> void
       def initialize(
         requested_constants:,
@@ -46,7 +48,8 @@ def initialize(
         app_root: ".",
         halt_upon_load_error: true,
         compiler_options: {},
-        lsp_addon: false
+        lsp_addon: false,
+        max_diff_lines: DEFAULT_MAX_DIFF_LINES
       )
         @requested_constants = requested_constants
         @requested_paths = requested_paths
@@ -66,6 +69,7 @@ def initialize(
         @skip_constant = skip_constant
         @compiler_options = compiler_options
         @lsp_addon = lsp_addon
+        @max_diff_lines = max_diff_lines
 
         super()
       end
@@ -247,7 +251,7 @@ def compile_dsl_rbi(constant_name, rbi, outpath: @outpath, quiet: false)
       def perform_dsl_verification(dir)
         diff = verify_dsl_rbi(tmp_dir: dir)
 
-        report_diff_and_exit_if_out_of_date(diff, :dsl)
+        report_diff_and_exit_if_out_of_date(diff, tmp_dir: dir, command: :dsl)
       ensure
         FileUtils.remove_entry(dir)
       end
@@ -269,7 +273,7 @@ def dsl_rbi_filename(constant_name)
         @outpath / "#{underscore(constant_name)}.rbi"
       end
 
-      #: (tmp_dir: Pathname) -> Hash[String, Symbol]
+      #: (tmp_dir: Pathname) -> Hash[Pathname, Symbol]
       def verify_dsl_rbi(tmp_dir:)
         diff = {}
 
@@ -301,7 +305,7 @@ def verify_dsl_rbi(tmp_dir:)
         diff
       end
 
-      #: (Symbol cause, Array[String] files) -> String
+      #: (Symbol cause, Array[Pathname] files) -> String
       def build_error_for_files(cause, files)
         filenames = files.map do |file|
           @outpath / file
@@ -310,26 +314,71 @@ def build_error_for_files(cause, files)
         "  File(s) #{cause}:\n  - #{filenames}"
       end
 
-      #: (Hash[String, Symbol] diff, Symbol command) -> void
-      def report_diff_and_exit_if_out_of_date(diff, command)
+      #: (Hash[Pathname, Symbol] diff, tmp_dir: Pathname, command: Symbol) -> void
+      def report_diff_and_exit_if_out_of_date(diff, tmp_dir:, command:)
         if diff.empty?
           say("Nothing to do, all RBIs are up-to-date.")
-        else
-          reasons = diff.group_by(&:last).sort.map do |cause, diff_for_cause|
-            build_error_for_files(cause, diff_for_cause.map(&:first))
-          end.join("\n")
+          return
+        end
+
+        reasons = diff.group_by(&:last).sort.map do |cause, diff_for_cause|
+          build_error_for_files(cause, diff_for_cause.map(&:first))
+        end.join("\n")
+
+        diff_output, diff_truncated = build_diff_output(diff, tmp_dir)
+
+        diff_section =
+          if diff_truncated
+            "#{set_color("Diff truncated to #{@max_diff_lines} lines.\nChange the number of displayed lines using the `--max-diff-lines` flag.", :red)}\n#{diff_output.rstrip}"
+          elsif !diff_output.empty?
+            "#{set_color("Diff:", :red)}\n#{diff_output.rstrip}"
+          else
+            ""
+          end
+
+        raise Tapioca::Error, <<~ERROR.rstrip
+          #{set_color("RBI files are out-of-date. In your development environment, please run:", :green)}
+            #{set_color("`#{default_command(command)}`", :green, :bold)}
+          #{set_color("Once it is complete, be sure to commit and push any changes", :green)}
+          If you don't observe any changes after running the command locally, ensure your database is in a good
+          state e.g. run `bin/rails db:reset`
 
-          raise Tapioca::Error, <<~ERROR
-            #{set_color("RBI files are out-of-date. In your development environment, please run:", :green)}
-              #{set_color("`#{default_command(command)}`", :green, :bold)}
-            #{set_color("Once it is complete, be sure to commit and push any changes", :green)}
-            If you don't observe any changes after running the command locally, ensure your database is in a good
-            state e.g. run `bin/rails db:reset`
+          #{set_color("Reason:", :red)}
+          #{reasons}
 
-            #{set_color("Reason:", :red)}
-            #{reasons}
-          ERROR
+          #{diff_section}
+        ERROR
+      end
+
+      #: (Hash[Pathname, Symbol] diff, Pathname tmp_dir) -> [String, bool]
+      def build_diff_output(diff, tmp_dir)
+        out = String.new
+        truncated = false #: bool
+        line_count = 0
+
+        diff.each do |file, status|
+          old_path = (@outpath / file)
+          new_path = (tmp_dir / file)
+
+          chunk = case status
+          when :added then file_diff(file, File::NULL, new_path)
+          when :removed then file_diff(file, old_path, File::NULL)
+          when :changed then file_diff(file, old_path, new_path)
+          else ""
+          end
+
+          remaining_lines = @max_diff_lines - line_count
+          if chunk.lines.count > remaining_lines
+            out << chunk.lines.first(remaining_lines).join
+            truncated = true
+            break
+          end
+
+          out << chunk
+          line_count += chunk.lines.count
         end
+
+        [out, truncated]
       end
 
       #: (Pathname path) -> Array[Pathname]

lib/tapioca/helpers/file_helper.rb

@@ -0,0 +1,42 @@
+# typed: strict
+# frozen_string_literal: true
+
+require "open3"
+
+module Tapioca
+  module FileHelper
+    #: (Pathname filename, Pathname | String old_path, Pathname | String new_path) -> String
+    def file_diff(filename, old_path, new_path)
+      filename = filename.to_s
+      stdout, stderr, status = Open3.capture3(
+        "diff",
+        "-u",
+        "--label=Current #{filename}",
+        old_path.to_s,
+        "--label=Correct #{filename} (After running `bin/tapioca dsl`)",
+        new_path.to_s,
+      )
+
+      unless [0, 1].include?(status.exitstatus)
+        error_msg("Failed to create #{filename} diff. #{stderr.chomp}")
+        return ""
+      end
+
+      stdout
+    rescue SystemCallError => e
+      error_msg("Failed to create #{filename} diff. #{e.message}")
+      ""
+    end
+
+    private
+
+    RED = "\e[31m" #: String
+    CLEAR = "\e[0m" #: String
+
+    #: (String message) -> void
+    def error_msg(message)
+      message = "#{RED}#{message}#{CLEAR}"
+      Kernel.warn(message)
+    end
+  end
+end

lib/tapioca/internal.rb

@@ -54,6 +54,7 @@
 require "tapioca/helpers/package_url"
 require "tapioca/helpers/cli_helper"
 require "tapioca/helpers/config_helper"
+require "tapioca/helpers/file_helper"
 require "tapioca/helpers/rbi_files_helper"
 require "tapioca/helpers/env_helper"