Display line diffs in dsl --verify error messages

[dejmedus]

Motivation

When tapioca 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 visualize these changes by including the line diff output in the error message

Output example

Implementation

This PR diffs updated files against the originals during RBI verification and then includes the output in the error message when the diff is 250 lines or less, or whatever value is provided to --max-diff-lines flag

Tests

• updated verify tests to include line diff
• added a test for multi file changes and changes longer than max diff lines
• added tests for diff behaviour

[KaanOzkan]

Should this 250 limit be a CLI option instead and default to 250? dsl --verify --max-diff-lines=1000

[dejmedus]

Great idea 👌🏼

[KaanOzkan]

Why did you opt for yellow and not red? I think red is more consistent with rest of our errors.

[dejmedus]

I looked through say_error uses and it seemed like :yellow was used for warnings and :red for errors. I leaned towards this being a warning. Makes sense, changed to :red :)

[KaanOzkan]

Does DslVerify write something to the file system? I think other DslVerify tests run on a temporary directory that's cleaned up but this is not an integration test so it shouldn't use that, it's more costly.

If it is writing something we should clean up after, maybe we can create @tmp = Dir.mktmpdir in the before block, use that as the outpath and then clean it up in an after block.

[dejmedus]

If im understanding correctly, outpath is required to create DslVerify but the command isn't being executed here, I just need an instance to pull file_diff from. I changed it to /dev/null to make it clearer that its unused. tbh I wouldn't be that against removing this test file if we don't find it helpful, file diff is being tested by verify tests also

[dejmedus]

Actually ignore above, I moved out file_diff to rbi_files_helpers and the tests look much nicer now :)

[KaanOzkan]

Sorry I'm suggesting a refactor that I didn't noticed before.

[KaanOzkan]

Instead of skipping it completely should we display the diff until @max_diff_lines?

[KaanOzkan]

Using exceptions for control flow is frowned upon and slow. So we shouldn't rely on it when we can just call say_error here instead. If Open3 itself can raise while running keep the rescue block too.

Suggested change

	Kernel.raise stderr.chomp unless [0, 1].include?(status.exitstatus)
	say_error("Failed to create #{filename} diff. #{e.message}", :red) unless [0, 1].include?(status.exitstatus)

[dejmedus]

Using exceptions for control flow is frowned upon and slow

Noted, thank you !

[KaanOzkan]

I don't love the tuple return here and I also forgot about report_diff_and_exit_if_out_of_date. What do you think about moving the calculation of diff output to report_diff_and_exit_if_out_of_date? It's the same class but it'll move some logic away from perform_dsl_verification/verify_dsl_rbi and make it easier to read. Something like this:

  #: (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.")
      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
        set_color("Diff not displayed as it exceeds #{@max_diff_lines} lines", :red)
      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`

      #{set_color("Reason:", :red)}
      #{reasons}

      #{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|
      chunk = case status
      when :added   then file_diff(file, File::NULL, tmp_dir / file)
      when :removed then file_diff(file, @outpath / file, File::NULL)
      when :changed then file_diff(file, @outpath / file, tmp_dir / file)
      end

      out << chunk
      line_count += chunk.count("\n")
      break if line_count > @max_diff_lines
    end

    out
  end

[dejmedus]

Agree, changed. Thanks!

[KaanOzkan]

Instead of this could we get away with defining our own say_error method? It's simpler if that's all we need.

[dejmedus]

We could, but we'd have to turn off type check in the file since files helpers require Thor as ancestor