Pull request formatter: Find position in full diff and fix commit_id

https://developer.github.com/v3/pulls/comments/#create-a-comment
appears to be misleading or wrong, depending on how you interpret it.

commit_id is meant to reference the "state" of the PR when creating the
commit, that is it references the most recent commit of the PR, always.

position is meant to be the position in the full diff, not the position
in any single commit's diff.

With full diff the combined diff of all commits is meant, as you get for
example via /:owner/:repo/pull/:id.diff

lib/pronto.rb

@@ -34,7 +34,7 @@ def self.run(commit = 'master', repo_path = '.',
 
     result = run_all_runners(patches)
 
-    formatted = formatter.format(result, repo)
+    formatted = formatter.format(result, repo, patches)
     puts formatted if formatted
 
     result

lib/pronto/formatter/checkstyle_formatter.rb

@@ -7,7 +7,7 @@ def initialize
         @output = ''
       end
 
-      def format(messages, _)
+      def format(messages, _, _)
         open_xml
         process_messages(messages)
         close_xml

lib/pronto/formatter/github_formatter.rb

@@ -1,7 +1,7 @@
 module Pronto
   module Formatter
     class GithubFormatter
-      def format(messages, repo)
+      def format(messages, repo, _)
         messages = messages.uniq { |message| [message.msg, message.line.new_lineno] }
         client = Github.new(repo)
 

lib/pronto/formatter/github_pull_request_formatter.rb

@@ -1,24 +1,17 @@
 module Pronto
   module Formatter
     class GithubPullRequestFormatter
-      def format(messages, repo)
+      def format(messages, repo, patches)
         messages = messages.uniq { |message| [message.msg, message.line.new_lineno] }
         client = Github.new(repo)
+        head = repo.head_commit_sha
 
         commit_messages = messages.map do |message|
           body = message.msg
           path = message.path
+          line = patches.find_line(message.full_path, message.line.new_lineno)
 
-          commits = repo.commits_until(message.commit_sha)
-
-          line = nil
-          sha = commits.find do |commit|
-            patches = repo.show_commit(commit)
-            line = patches.find_line(message.full_path, message.line.new_lineno)
-            line
-          end
-
-          create_comment(client, sha, body, path, line.position)
+          create_comment(client, head, body, path, line.position)
         end
 
         "#{commit_messages.compact.count} Pronto messages posted to GitHub"

lib/pronto/formatter/gitlab_formatter.rb

@@ -1,7 +1,7 @@
 module Pronto
   module Formatter
     class GitlabFormatter
-      def format(messages, repo)
+      def format(messages, repo, _)
         messages = messages.uniq { |message| [message.msg, message.line.new_lineno] }
         client = Gitlab.new repo
 

lib/pronto/formatter/json_formatter.rb

@@ -3,7 +3,7 @@
 module Pronto
   module Formatter
     class JsonFormatter
-      def format(messages, _)
+      def format(messages, _, _)
         messages.map do |message|
           lineno = message.line.new_lineno if message.line
 

lib/pronto/formatter/null_formatter.rb

@@ -1,7 +1,7 @@
 module Pronto
   module Formatter
     class NullFormatter
-      def format(_, _)
+      def format(_, _, _)
       end
     end
   end

lib/pronto/formatter/text_formatter.rb

@@ -1,7 +1,7 @@
 module Pronto
   module Formatter
     class TextFormatter
-      def format(messages, _)
+      def format(messages, _, _)
         messages.map do |message|
           level = message.level[0].upcase
           line = message.line

lib/pronto/git/repository.rb

@@ -58,6 +58,10 @@ def remote_urls
         @repo.remotes.map(&:url)
       end
 
+      def head_commit_sha
+        head.oid
+      end
+
       private
 
       def empty_patches(sha)

spec/pronto/formatter/checkstyle_formatter_spec.rb

@@ -6,7 +6,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, nil) }
+        subject { formatter.format(messages, nil, nil) }
         let(:line) { double(new_lineno: 1, commit_sha: '123') }
         let(:error) { Message.new('path/to', line, :error, 'Line Error') }
         let(:warning) { Message.new('path/to', line, :warning, 'Line Warning') }

spec/pronto/formatter/github_formattter_spec.rb

@@ -6,7 +6,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, repo) }
+        subject { formatter.format(messages, repo, nil) }
         let(:messages) { [message, message] }
         let(:repo) { Git::Repository.new('spec/fixtures/test.git') }
         let(:message) { Message.new('path/to', line, :warning, 'crucial') }
@@ -28,7 +28,7 @@ module Formatter
       end
 
       describe '#format without duplicates' do
-        subject { formatter.format(messages, repo) }
+        subject { formatter.format(messages, repo, nil) }
         let(:messages) { [message1, message2] }
         let(:repo) { Git::Repository.new('spec/fixtures/test.git') }
         let(:message1) { Message.new('path/to1', line1, :warning, 'crucial') }

spec/pronto/formatter/github_pull_request_formattter_spec.rb

@@ -8,11 +8,12 @@ module Formatter
       let(:repo) { Git::Repository.new('spec/fixtures/test.git') }
 
       describe '#format' do
-        subject { formatter.format(messages, repo) }
+        subject { formatter.format(messages, repo, patches) }
         let(:messages) { [message, message] }
         let(:message) { Message.new(patch.new_file_full_path, line, :warning, 'crucial') }
         let(:patch) { repo.show_commit('64dadfd').first }
         let(:line) { patch.added_lines.first }
+        let(:patches) { repo.diff('64dadfd^') }
 
         specify do
           ENV['PULL_REQUEST_ID'] = '10'

spec/pronto/formatter/gitlab_formatter_spec.rb

@@ -9,7 +9,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, repo) }
+        subject { formatter.format(messages, repo, nil) }
         let(:messages) { [message, message] }
         let(:repo) { Git::Repository.new('spec/fixtures/test.git') }
         let(:message) { Message.new('path/to', line, :warning, 'crucial') }
@@ -31,7 +31,7 @@ module Formatter
       end
 
       describe '#format without duplicates' do
-        subject { formatter.format(messages, repo) }
+        subject { formatter.format(messages, repo, nil) }
         let(:messages) { [message1, message2] }
         let(:repo) { Git::Repository.new('spec/fixtures/test.git') }
         let(:message1) { Message.new('path/to1', line1, :warning, 'crucial') }

spec/pronto/formatter/json_formatter_spec.rb

@@ -6,7 +6,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, nil) }
+        subject { formatter.format(messages, nil, nil) }
         let(:messages) { [message, message] }
         let(:message) { Message.new('path/to', line, :warning, 'crucial') }
         let(:line) { double(new_lineno: 1, commit_sha: nil) }

spec/pronto/formatter/null_formatter_spec.rb

@@ -6,7 +6,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, nil) }
+        subject { formatter.format(messages, nil, nil) }
         let(:messages) { [message, message] }
         let(:message) { Message.new('path/to', line, :warning, 'crucial') }
         let(:line) { double(new_lineno: 1, commit_sha: '123') }

spec/pronto/formatter/text_formatter_spec.rb

@@ -6,7 +6,7 @@ module Formatter
       let(:formatter) { described_class.new }
 
       describe '#format' do
-        subject { formatter.format(messages, nil) }
+        subject { formatter.format(messages, nil, nil) }
         let(:messages) { [message, message] }
         let(:message) { Message.new('path/to', line, :warning, 'crucial') }
         let(:line) { double(new_lineno: 1, commit_sha: '123') }