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