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