Merge pull request #1 from reclaim-the-stack/diff-handling

Add local diff handling

lib/github.rb

@@ -0,0 +1,80 @@
+require "json"
+require "net/http"
+
+module Github
+  HttpError = Class.new(StandardError)
+
+  class File
+    RANGE_INFORMATION_LINE = /^@@ .+\+(?<line_number>\d+),/.freeze
+    MODIFIED_LINE = /^\+(?!\+|\+)/.freeze
+    NOT_REMOVED_LINE = /^[^-]/.freeze
+
+    def initialize(file)
+      @file = file
+    end
+
+    def path
+      @file.fetch("filename")
+    end
+
+    def changed_lines
+      patch = @file.fetch("patch") || ""
+      line_number = 0
+      patch.each_line.with_object([]) do |line_content, lines|
+        case line_content
+        when RANGE_INFORMATION_LINE
+          line_number = Regexp.last_match[:line_number].to_i
+        when MODIFIED_LINE
+          lines << line_number
+          line_number += 1
+        when NOT_REMOVED_LINE
+          line_number += 1
+        end
+      end
+    end
+  end
+
+  CONNECTION = Net::HTTP.new("api.github.com", 443).tap { |http| http.use_ssl = true }
+  REQUEST_METHOD_TO_CLASS = {
+    get: Net::HTTP::Get,
+    patch: Net::HTTP::Patch,
+    post: Net::HTTP::Post,
+    delete: Net::HTTP::Delete,
+  }.freeze
+
+  REQUEST_METHOD_TO_CLASS.each do |method, klass|
+    define_singleton_method(method) do |path, params = nil|
+      request(klass, path, params)
+    end
+
+    define_singleton_method("#{method}!") do |path, params = nil|
+      response = request(klass, path, params)
+      raise HttpError, "status: #{response.code}, body: #{response.body}" unless response.is_a?(Net::HTTPSuccess)
+
+      JSON.parse(response.body) if response.body
+    end
+  end
+
+  def self.pull_request_ruby_files(owner_and_repository, pr_number)
+    changed_files = []
+    1.step do |page|
+      files = Github.get!("/repos/#{owner_and_repository}/pulls/#{pr_number}/files?per_page=100&page=#{page}")
+      changed_files.concat(files)
+      break if files.length < 100
+    end
+    changed_files
+      .reject { |file| file.fetch("status") == "removed" }
+      .select { |file| file.fetch("filename").end_with?(".rb") }
+      .map { |file| File.new(file) }
+  end
+
+  def self.request(request_class, path, params = nil)
+    request = request_class.new(path)
+    request.content_type = "application/json"
+    request.body = params&.to_json
+    request["Authorization"] = "Bearer #{ENV.fetch('GITHUB_TOKEN')}"
+    request["Accept"] = "application/vnd.github.v3+json"
+
+    CONNECTION.request(request)
+  end
+end

rubocop.rb

@@ -23,70 +23,23 @@
 
 # Script
 
-require "json"
-require "net/http"
-
-module Github
-  HttpError = Class.new(StandardError)
-
-  CONNECTION = Net::HTTP.new("api.github.com", 443).tap { |http| http.use_ssl = true }
-  REQUEST_METHOD_TO_CLASS = {
-    get: Net::HTTP::Get,
-    patch: Net::HTTP::Patch,
-    post: Net::HTTP::Post,
-    delete: Net::HTTP::Delete,
-  }.freeze
-
-  REQUEST_METHOD_TO_CLASS.each do |method, klass|
-    define_singleton_method(method) do |path, params = nil|
-      request(klass, path, params)
-    end
-
-    define_singleton_method("#{method}!") do |path, params = nil|
-      response = request(klass, path, params)
-      raise HttpError, "status: #{response.code}, body: #{response.body}" unless response.is_a?(Net::HTTPSuccess)
-
-      JSON.parse(response.body) if response.body
-    end
-  end
-
-  def self.request(request_class, path, params = nil)
-    request = request_class.new(path)
-    request.content_type = "application/json"
-    request.body = params&.to_json
-    request["Authorization"] = "Bearer #{ENV.fetch('GITHUB_TOKEN')}"
-    request["Accept"] = "application/vnd.github.v3+json"
-
-    CONNECTION.request(request)
-  end
-end
+require_relative "lib/github"
 
 # Figure out which ruby files have changed and run Rubocop on them
 
 github_event = JSON.parse(File.read(ENV.fetch("GITHUB_EVENT_PATH")))
 pr_number = github_event.fetch("pull_request").fetch("number")
 owner_and_repository = ENV.fetch("GITHUB_REPOSITORY")
 
-changed_files = []
-1.step do |page|
-  files = Github.get!("/repos/#{owner_and_repository}/pulls/#{pr_number}/files?per_page=100&page=#{page}")
-  changed_files.concat(files)
-  break if files.length < 100
-end
-changed_ruby_files = changed_files
-  .reject { |file| file.fetch("status") == "removed" }
-  .select { |file| file.fetch("filename").end_with?(".rb") }
-  .map { |file| file.fetch("filename") }
+changed_ruby_files = Github.pull_request_ruby_files(owner_and_repository, pr_number)
 
 # JSON reference: https://docs.rubocop.org/rubocop/formatters.html#json-formatter
 files_with_offenses =
   if changed_ruby_files.any?
-    command = "rubocop #{changed_ruby_files.join(' ')} --format json --force-exclusion"
+    command = "rubocop #{changed_ruby_files.map(&:path).join(' ')} --format json --force-exclusion"
 
     puts "Running rubocop with: #{command}"
-    rubocop_output = `#{command}`
-
-    JSON.parse(rubocop_output).fetch("files")
+    JSON.parse(`#{command}`).fetch("files")
   else
     puts "No changed Ruby files, skipping rubocop"
 
@@ -126,6 +79,11 @@ def self.request(request_class, path, params = nil)
 
 # Comment on the pull request with the offenses found
 
+def in_diff?(changed_files, path, line)
+  file = changed_files.find { |changed_file| changed_file.path == path }
+  file&.changed_lines&.include?(line)
+end
+
 offences_outside_diff = []
 
 files_with_offenses.each do |file|
@@ -166,26 +124,21 @@ def self.request(request_class, path, params = nil)
 
       puts "Updating comment #{comment_id} on #{path} line #{line}"
       Github.patch("/repos/#{owner_and_repository}/pulls/comments/#{comment_id}", body: body)
-    else
+    elsif in_diff?(changed_ruby_files, path, line)
       puts "Commenting on #{path} line #{line}"
 
       # Somehow the commit_id should not be just the HEAD SHA: https://stackoverflow.com/a/71431370/1075108
       commit_id = github_event.fetch("pull_request").fetch("head").fetch("sha")
 
-      response = Github.post(
+      Github.post!(
         "/repos/#{owner_and_repository}/pulls/#{pr_number}/comments",
         body: body,
         path: path,
         commit_id: commit_id,
         line: line,
       )
-
-      # Rubocop might hit errors on lines which are not part of the diff and thus cannot be commented on.
-      if response.code == "422" && response.body.include?("line must be part of the diff")
-        puts "Deferring comment on #{path} line #{line} because it isn't part of the diff"
-
-        offences_outside_diff << { path: path, line: line, message: message }
-      end
+    else
+      offences_outside_diff << { path: path, line: line, message: message }
     end
   end
 end
@@ -196,7 +149,9 @@ def self.request(request_class, path, params = nil)
 existing_separate_comment = separate_comments.find do |comment|
   comment.fetch("body").include?("rubocop-comment-id: outside-diff")
 end
+
 if offences_outside_diff.any?
+  puts "Found #{offences_outside_diff.count} offenses outside of the diff"
 
   body = <<~BODY
     <!-- rubocop-comment-id: outside-diff -->
@@ -215,8 +170,8 @@ def self.request(request_class, path, params = nil)
     if existing_separate_comment.fetch("body") == body
       puts "Skipping unchanged separate comment #{existing_comment_id}"
     else
-      puts "Updating comment #{existing_comment_id} on pull request"
-      Github.patch!("/repos/#{owner_and_repository}/pulls/comments/#{existing_comment_id}", body: body)
+      puts "Updating separate comment #{existing_comment_id}"
+      Github.patch!("/repos/#{owner_and_repository}/issues/comments/#{existing_comment_id}", body: body)
     end
   else
     puts "Commenting on pull request with offenses found outside the diff"
@@ -227,6 +182,8 @@ def self.request(request_class, path, params = nil)
   existing_comment_id = existing_separate_comment.fetch("id")
   puts "Deleting resolved separate comment #{existing_comment_id}"
   Github.delete("/repos/#{owner_and_repository}/issues/comments/#{existing_comment_id}")
+else
+  puts "No offenses found outside of the diff and no existing separate comment to remove"
 end
 
 # Fail the build if there were any offenses