From 85ec455af080b87962569f88dfbb1d397c4150dc Mon Sep 17 00:00:00 2001 From: Julia Boutin Date: Thu, 2 Apr 2026 16:51:11 -0600 Subject: [PATCH] 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 --- README.md | 3 + lib/tapioca.rb | 1 + lib/tapioca/cli.rb | 5 + lib/tapioca/commands/abstract_dsl.rb | 80 +++++-- lib/tapioca/helpers/rbi_files_helper.rb | 26 +++ spec/tapioca/cli/dsl_spec.rb | 217 +++++++++++++++++- spec/tapioca/helpers/rbi_files_helper_spec.rb | 96 ++++++++ 7 files changed, 407 insertions(+), 21 deletions(-) create mode 100644 spec/tapioca/helpers/rbi_files_helper_spec.rb diff --git a/README.md b/README.md index eaca2750b..370a3cae7 100644 --- a/README.md +++ b/README.md @@ -502,6 +502,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 @@ -960,6 +962,7 @@ dsl: quiet: false workers: 1 rbi_max_line_length: 120 + max_diff_lines: 250 environment: development list_compilers: false app_root: "." diff --git a/lib/tapioca.rb b/lib/tapioca.rb index 8e47d3b03..5eafd9ce6 100644 --- a/lib/tapioca.rb +++ b/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" diff --git a/lib/tapioca/cli.rb b/lib/tapioca/cli.rb index fc52f39ec..439f9237b 100644 --- a/lib/tapioca/cli.rb +++ b/lib/tapioca/cli.rb @@ -116,6 +116,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, @@ -166,6 +170,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] diff --git a/lib/tapioca/commands/abstract_dsl.rb b/lib/tapioca/commands/abstract_dsl.rb index d52960565..ae97b9aab 100644 --- a/lib/tapioca/commands/abstract_dsl.rb +++ b/lib/tapioca/commands/abstract_dsl.rb @@ -26,7 +26,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 +47,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 +68,7 @@ def initialize( @skip_constant = skip_constant @compiler_options = compiler_options @lsp_addon = lsp_addon + @max_diff_lines = max_diff_lines super() end @@ -242,7 +245,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 @@ -305,26 +308,67 @@ 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[String, 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 = build_diff_output(diff, tmp_dir) + diff_lines = diff_output.count("\n") + + diff_section = + if diff_lines.between?(1, @max_diff_lines) + "#{set_color("Diff:", :red)}\n#{diff_output.chomp}" + elsif diff_lines > @max_diff_lines + truncated_output = diff_output.lines.first(@max_diff_lines).join + "#{set_color("Diff truncated to #{@max_diff_lines} lines:", :red)}\n#{truncated_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[String, Symbol] diff, Pathname tmp_dir) -> String + def build_diff_output(diff, tmp_dir) + out = String.new + line_count = 0 + + diff.each do |file, status| + filename = file.to_s + old_path = (@outpath / file).to_s + new_path = (tmp_dir / file).to_s + + chunk = case status + when :added then file_diff(filename, File::NULL, new_path) + when :removed then file_diff(filename, old_path, File::NULL) + when :changed then file_diff(filename, old_path, new_path) + else "" + end + + out << chunk + line_count += chunk.count("\n") + break if line_count > @max_diff_lines end + + out end #: (Pathname path) -> Array[Pathname] diff --git a/lib/tapioca/helpers/rbi_files_helper.rb b/lib/tapioca/helpers/rbi_files_helper.rb index b84e81832..a8ae3a0ac 100644 --- a/lib/tapioca/helpers/rbi_files_helper.rb +++ b/lib/tapioca/helpers/rbi_files_helper.rb @@ -1,6 +1,8 @@ # typed: strict # frozen_string_literal: true +require "open3" + module Tapioca # @requires_ancestor: Thor::Shell # @requires_ancestor: SorbetHelper @@ -137,6 +139,30 @@ def validate_rbi_files(command:, gem_dir:, dsl_dir:, auto_strictness:, gems: [], Kernel.raise Tapioca::Error, error_messages.join("\n") if parse_errors.any? end + #: (String filename, String old_path, String new_path) -> String + def file_diff(filename, old_path, new_path) + stdout, stderr, status = Open3.capture3( + "diff", + "-u", + "--label", + filename, + "--label", + filename, + old_path, + new_path, + ) + + unless [0, 1].include?(status.exitstatus) + say_error("Failed to create #{filename} diff. #{stderr.chomp}", :red) + return "" + end + + stdout + rescue => e + say_error("Failed to create #{filename} diff. #{e.message}", :red) + "" + end + private #: (RBI::Index index, Array[String] files, number_of_workers: Integer?) -> void diff --git a/spec/tapioca/cli/dsl_spec.rb b/spec/tapioca/cli/dsl_spec.rb index 909a437a1..87fc71900 100644 --- a/spec/tapioca/cli/dsl_spec.rb +++ b/spec/tapioca/cli/dsl_spec.rb @@ -1934,13 +1934,28 @@ def perform(foo, bar) after do @project.remove!("sorbet/rbi/dsl") + @project.remove!("lib/image.rb") + @project.write!("lib/post.rb", <<~RB) + require "smart_properties" + + class Post + include SmartProperties + property :title, accepts: String + end + RB end it "does nothing and returns exit status 0 with no changes" do @project.tapioca("dsl") result = @project.tapioca("dsl --verify") - assert_stdout_includes(result, <<~OUT) + assert_stdout_equals(<<~OUT, result) + Loading DSL extension classes... Done + Loading Rails application... Done + Loading DSL compiler classes... Done + Checking for out-of-date RBIs... + + Nothing to do, all RBIs are up-to-date. OUT @@ -1971,6 +1986,29 @@ def perform(foo, bar) Reason: File(s) removed: - sorbet/rbi/dsl/post.rbi + + Diff: + --- post.rbi + +++ post.rbi + @@ -1,18 +0,0 @@ + -# typed: true + - + -# DO NOT EDIT MANUALLY + -# This is an autogenerated file for dynamic methods in `Post`. + -# Please instead update this file by running `bin/tapioca dsl Post`. + - + - + -class Post + - include SmartPropertiesGeneratedMethods + - + - module SmartPropertiesGeneratedMethods + - sig { returns(T.nilable(::String)) } + - def title; end + - + - sig { params(title: T.nilable(::String)).returns(T.nilable(::String)) } + - def title=(title); end + - end + -end ERROR refute_success_status(result) @@ -2010,11 +2048,32 @@ class Image Reason: File(s) added: - sorbet/rbi/dsl/image.rbi + + Diff: + --- image.rbi + +++ image.rbi + @@ -0,0 +1,18 @@ + +# typed: true + + + +# DO NOT EDIT MANUALLY + +# This is an autogenerated file for dynamic methods in `Image`. + +# Please instead update this file by running `bin/tapioca dsl Image`. + + + + + +class Image + + include SmartPropertiesGeneratedMethods + + + + module SmartPropertiesGeneratedMethods + + sig { returns(T.nilable(::String)) } + + def title; end + + + + sig { params(title: T.nilable(::String)).returns(T.nilable(::String)) } + + def title=(title); end + + end + +end ERROR refute_success_status(result) - - @project.remove!("lib/image.rb") end it "advises of modified file(s) and returns exit status 1 with modified file" do @@ -2060,10 +2119,162 @@ class Post Reason: File(s) changed: - sorbet/rbi/dsl/post.rbi + + Diff: + --- post.rbi + +++ post.rbi + @@ -10,6 +10,12 @@ + #{" "} + module SmartPropertiesGeneratedMethods + sig { returns(T.nilable(::String)) } + + def desc; end + + + + sig { params(desc: T.nilable(::String)).returns(T.nilable(::String)) } + + def desc=(desc); end + + + + sig { returns(T.nilable(::String)) } + def title; end + #{" "} + sig { params(title: T.nilable(::String)).returns(T.nilable(::String)) } + ERROR + + refute_success_status(result) + end + + it "advises of added and changed files and returns exit status 1" do + @project.tapioca("dsl") + + @project.write!("lib/post.rb", <<~RB) + require "smart_properties" + + class Post + include SmartProperties + property :title, accepts: String + property :body, accepts: String + end + RB + + @project.write!("lib/image.rb", <<~RB) + require "smart_properties" + + class Image + include(SmartProperties) + + property :title, accepts: String + end + RB + + result = @project.tapioca("dsl --verify") + + assert_stdout_equals(<<~OUT, result) + Loading DSL extension classes... Done + Loading Rails application... Done + Loading DSL compiler classes... Done + Checking for out-of-date RBIs... + + + OUT + + assert_stderr_equals(<<~ERROR, result) + RBI files are out-of-date. In your development environment, please run: + `bin/tapioca dsl` + Once it is complete, be sure to commit and push any changes + 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` + + Reason: + File(s) added: + - sorbet/rbi/dsl/image.rbi + File(s) changed: + - sorbet/rbi/dsl/post.rbi + + Diff: + --- image.rbi + +++ image.rbi + @@ -0,0 +1,18 @@ + +# typed: true + + + +# DO NOT EDIT MANUALLY + +# This is an autogenerated file for dynamic methods in `Image`. + +# Please instead update this file by running `bin/tapioca dsl Image`. + + + + + +class Image + + include SmartPropertiesGeneratedMethods + + + + module SmartPropertiesGeneratedMethods + + sig { returns(T.nilable(::String)) } + + def title; end + + + + sig { params(title: T.nilable(::String)).returns(T.nilable(::String)) } + + def title=(title); end + + end + +end + --- post.rbi + +++ post.rbi + @@ -10,6 +10,12 @@ + #{" "} + module SmartPropertiesGeneratedMethods + sig { returns(T.nilable(::String)) } + + def body; end + + + + sig { params(body: T.nilable(::String)).returns(T.nilable(::String)) } + + def body=(body); end + + + + sig { returns(T.nilable(::String)) } + def title; end + #{" "} + sig { params(title: T.nilable(::String)).returns(T.nilable(::String)) } ERROR refute_success_status(result) end + + it "truncates diff output to 250 lines when it exceeds the limit" do + @project.tapioca("dsl") + + @project.write!("lib/post.rb", <<~RB) + require "smart_properties" + + class Post + include SmartProperties + #{(1..250).map { |i| "property :p#{i}, accepts: String" }.join("\n")} + end + RB + + result = @project.tapioca("dsl --verify") + + assert_stderr_includes(result, "Diff truncated to 250 lines:") + assert_stderr_includes(result, "--- post.rbi") + + diff_section = result.err.to_s.partition("Diff truncated to 250 lines:\n").last + assert_equal(250, diff_section.lines.count) + + refute_success_status(result) + end + + it "respects a custom --max-diff-lines value" do + @project.tapioca("dsl") + + @project.write!("lib/post.rb", <<~RB) + require "smart_properties" + + class Post + include SmartProperties + property :title, accepts: String + property :desc, accepts: String + end + RB + + result = @project.tapioca("dsl --verify --max-diff-lines=5") + + assert_stderr_includes(result, "Diff truncated to 5 lines:") + + diff_section = result.err.to_s.partition("Diff truncated to 5 lines:\n").last + assert_equal(5, diff_section.lines.count) + + refute_success_status(result) + end end describe "strictness" do diff --git a/spec/tapioca/helpers/rbi_files_helper_spec.rb b/spec/tapioca/helpers/rbi_files_helper_spec.rb new file mode 100644 index 000000000..570b2b87b --- /dev/null +++ b/spec/tapioca/helpers/rbi_files_helper_spec.rb @@ -0,0 +1,96 @@ +# typed: true +# frozen_string_literal: true + +require "spec_helper" + +# RBI file helpers require Thor as an ancestor. However, including Thor mutates +# the including class's initialize method to expect Thor command args, which trips +# up Minitest's test creation. Since we just want to test the helper methods, we +# can bypass Thor's initialize method and create tests directly +module TestFriendlyThor + include Thor::Shell + + def initialize(name) + Minitest::Runnable.instance_method(:initialize).bind(self).call(name) + end +end + +class Tapioca::RBIFilesHelperSpec < Minitest::Spec + include TestFriendlyThor + include Tapioca::SorbetHelper + include Tapioca::RBIFilesHelper + + describe "#file_diff" do + it "returns diff when files differ" do + Dir.mktmpdir do |dir| + a = File.join(dir, "a") + b = File.join(dir, "b") + File.write(a, "line1\nline2\n") + File.write(b, "line1\nline3\n") + + result = file_diff("x.rbi", a, b) + + assert_includes(result, "--- x.rbi") + assert_includes(result, "+++ x.rbi") + assert_includes(result, "-line2") + assert_includes(result, "+line3") + end + end + + it "returns empty string when files are identical" do + Dir.mktmpdir do |dir| + a = File.join(dir, "a") + File.write(a, "line1\n") + + assert_equal("", file_diff("x.rbi", a, a)) + end + end + + it "diffs against null path when a file is added" do + Dir.mktmpdir do |dir| + a = File.join(dir, "a") + File.write(a, "line1\n") + + result = file_diff("x.rbi", File::NULL, a) + + assert_includes(result, "+++ x.rbi") + assert_includes(result, "+line1") + end + end + + it "returns empty string when file path is missing" do + Dir.mktmpdir do |dir| + a = File.join(dir, "a") + File.write(a, "x\n") + + _out, err = capture_io do + assert_equal("", file_diff("x.rbi", a, "/nonexistent/path")) + end + + assert_match(/Failed to create x\.rbi diff\./, err) + end + end + + it "returns empty string when diff is unavailable" do + _out, err = capture_io do + Open3.stub(:capture3, ->(*_args) { raise Errno::ENOENT, "diff" }) do + assert_equal("", file_diff("x.rbi", "/a", "/b")) + end + end + + assert_match(/Failed to create x\.rbi diff\./, err) + end + + it "returns empty string when the process has no exitstatus" do + fake_status = Struct.new(:exitstatus).new(nil) + + _out, err = capture_io do + Open3.stub(:capture3, ->(*_args) { ["", "", fake_status] }) do + assert_equal("", file_diff("x.rbi", "/a", "/b")) + end + end + + assert_match(/Failed to create x\.rbi diff\./, err) + end + end +end