From b7963a2048133b14c8b73fcb1456bace2ddb526e 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 | 9 + lib/tapioca/commands/abstract_dsl.rb | 89 +++++++-- lib/tapioca/helpers/file_helper.rb | 42 ++++ lib/tapioca/internal.rb | 1 + spec/tapioca/cli/dsl_spec.rb | 237 ++++++++++++++++++++++- spec/tapioca/helpers/file_helper_spec.rb | 96 +++++++++ 8 files changed, 455 insertions(+), 23 deletions(-) create mode 100644 lib/tapioca/helpers/file_helper.rb create mode 100644 spec/tapioca/helpers/file_helper_spec.rb diff --git a/README.md b/README.md index 159bb6733..dd107435c 100644 --- a/README.md +++ b/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: "." 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 977f64165..a46468f27 100644 --- a/lib/tapioca/cli.rb +++ b/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] diff --git a/lib/tapioca/commands/abstract_dsl.rb b/lib/tapioca/commands/abstract_dsl.rb index a9dbd1469..773678463 100644 --- a/lib/tapioca/commands/abstract_dsl.rb +++ b/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] diff --git a/lib/tapioca/helpers/file_helper.rb b/lib/tapioca/helpers/file_helper.rb new file mode 100644 index 000000000..b83d8c50c --- /dev/null +++ b/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 diff --git a/lib/tapioca/internal.rb b/lib/tapioca/internal.rb index a77eb7c1f..d7fa4bbda 100644 --- a/lib/tapioca/internal.rb +++ b/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" diff --git a/spec/tapioca/cli/dsl_spec.rb b/spec/tapioca/cli/dsl_spec.rb index 382a85969..1dfde6f74 100644 --- a/spec/tapioca/cli/dsl_spec.rb +++ b/spec/tapioca/cli/dsl_spec.rb @@ -2116,13 +2116,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 @@ -2150,6 +2165,18 @@ def perform(foo, bar) refute_success_status(result) end + it "rejects negative --max-diff-lines values" do + ["0", "-1"].each do |value| + result = @project.tapioca("dsl --verify --max-diff-lines=#{value}") + + assert_stderr_includes( + result, + "Option '--max-diff-lines' must be a positive number", + ) + refute_success_status(result) + end + end + it "advises of removed file(s) and returns exit status 1 when files are excluded" do @project.tapioca("dsl") result = @project.tapioca("dsl --verify --exclude SmartProperties") @@ -2173,6 +2200,29 @@ def perform(foo, bar) Reason: File(s) removed: - sorbet/rbi/dsl/post.rbi + + Diff: + --- Current post.rbi + +++ Correct post.rbi (After running `bin/tapioca dsl`) + @@ -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) @@ -2212,11 +2262,32 @@ class Image Reason: File(s) added: - sorbet/rbi/dsl/image.rbi + + Diff: + --- Current image.rbi + +++ Correct image.rbi (After running `bin/tapioca dsl`) + @@ -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 @@ -2262,10 +2333,170 @@ class Post Reason: File(s) changed: - sorbet/rbi/dsl/post.rbi + + Diff: + --- Current post.rbi + +++ Correct post.rbi (After running `bin/tapioca dsl`) + @@ -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: + --- Current image.rbi + +++ Correct image.rbi (After running `bin/tapioca dsl`) + @@ -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 + --- Current post.rbi + +++ Correct post.rbi (After running `bin/tapioca dsl`) + @@ -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, "--- Current post.rbi") + + diff_section = result.err.to_s.partition(<<~MESSAGE).last + Diff truncated to 250 lines. + Change the number of displayed lines using the `--max-diff-lines` flag. + MESSAGE + diff_output = diff_section + assert_equal(250, diff_output.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(<<~MESSAGE).last + Diff truncated to 5 lines. + Change the number of displayed lines using the `--max-diff-lines` flag. + MESSAGE + diff_output = diff_section + assert_equal(5, diff_output.lines.count) + + refute_success_status(result) + end end describe "strictness" do diff --git a/spec/tapioca/helpers/file_helper_spec.rb b/spec/tapioca/helpers/file_helper_spec.rb new file mode 100644 index 000000000..203d6e494 --- /dev/null +++ b/spec/tapioca/helpers/file_helper_spec.rb @@ -0,0 +1,96 @@ +# typed: true +# frozen_string_literal: true + +require "spec_helper" + +class Tapioca::FileHelperSpec < Minitest::Spec + FILENAME = Pathname.new("x.rbi") + + include Tapioca::FileHelper + + describe "#file_diff" do + it "returns diff when files differ" do + Dir.mktmpdir do |dir| + dir = Pathname.new(dir) + a = dir / "a" + b = dir / "b" + a.write("line1\nline2\n") + b.write("line1\nline3\n") + + result = file_diff(FILENAME, a, b) + + assert_equal(<<~DIFF, result) + --- Current x.rbi + +++ Correct x.rbi (After running `bin/tapioca dsl`) + @@ -1,2 +1,2 @@ + line1 + -line2 + +line3 + DIFF + end + end + + it "returns empty string when files are identical" do + Dir.mktmpdir do |dir| + dir = Pathname.new(dir) + a = dir / "a" + a.write("line1\n") + + assert_equal("", file_diff(FILENAME, a, a)) + end + end + + it "diffs against null path when a file is added" do + Dir.mktmpdir do |dir| + dir = Pathname.new(dir) + a = dir / "a" + a.write("line1\n") + + result = file_diff(FILENAME, File::NULL, a) + + assert_equal(<<~DIFF, result) + --- Current x.rbi + +++ Correct x.rbi (After running `bin/tapioca dsl`) + @@ -0,0 +1 @@ + +line1 + DIFF + end + end + + it "returns empty string when file path is missing" do + Dir.mktmpdir do |dir| + dir = Pathname.new(dir) + a = dir / "a" + a.write("x\n") + + _out, err = capture_io do + assert_equal("", file_diff(FILENAME, 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(FILENAME, "/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(FILENAME, "/a", "/b")) + end + end + + assert_match(/Failed to create x\.rbi diff\./, err) + end + end +end