Remove usage of deprecated position argument for GitHub comments

lib/pronto/formatter/github_pull_request_formatter.rb

@@ -13,9 +13,8 @@ def pretty_name
         'GitHub'
       end
 
-      def line_number(message, patches)
-        line = patches.find_line(message.full_path, message.line.new_lineno)
-        line.position
+      def line_number(message, _)
+        message.line&.new_lineno
       end
     end
   end

lib/pronto/formatter/github_pull_request_review_formatter.rb

@@ -19,9 +19,8 @@ def submit_comments(client, comments)
         $stderr.puts "Failed to post: #{e.message}"
       end
 
-      def line_number(message, patches)
-        line = patches.find_line(message.full_path, message.line.new_lineno)
-        line.position
+      def line_number(message, _)
+        message.line&.new_lineno
       end
     end
   end

lib/pronto/github.rb

@@ -10,8 +10,9 @@ def initialize(repo)
     def pull_comments(sha)
       @comment_cache["#{pull_id}/#{sha}"] ||= begin
         client.pull_comments(slug, pull_id).map do |comment|
-          Comment.new(sha, comment.body, comment.path,
-                      comment.position || comment.original_position)
+          Comment.new(
+            sha, comment.body, comment.path, comment.line || comment.original_line
+          )
         end
       end
     rescue Octokit::NotFound => e
@@ -23,7 +24,7 @@ def pull_comments(sha)
     def commit_comments(sha)
       @comment_cache[sha.to_s] ||= begin
         client.commit_comments(slug, sha).map do |comment|
-          Comment.new(sha, comment.body, comment.path, comment.position)
+          Comment.new(sha, comment.body, comment.path, comment.line)
         end
       end
     end
@@ -37,9 +38,13 @@ def create_commit_comment(comment)
     def create_pull_comment(comment)
       if comment.path && comment.position
         @config.logger.log("Creating pull request comment on #{pull_id}")
-        client.create_pull_comment(slug, pull_id, comment.body,
-                                   pull_sha || comment.sha,
-                                   comment.path, comment.position)
+        client.create_pull_comment(
+          # Depending on the Octokit version the 6th argument can be either postion or line. We'll
+          # provide the `line` as this argument and also provide the line in the options argument.
+          # The API uses `line` and ignores position when `line` is provided.
+          slug, pull_id, comment.body, pull_sha || comment.sha,
+          comment.path, comment.position, { line: comment.position }
+        )
       else
         create_commit_comment(comment)
       end
@@ -66,12 +71,11 @@ def create_commit_status(status)
     def create_pull_request_review(comments)
       options = {
         event: @config.github_review_type,
-        accept: 'application/vnd.github.v3.diff+json', # https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review
         comments: comments.map do |comment|
           {
-            path:     comment.path,
-            position: comment.position,
-            body:     comment.body
+            path: comment.path,
+            line: comment.position,
+            body: comment.body
           }
         end
       }

pronto.gemspec

@@ -42,7 +42,7 @@ Gem::Specification.new do |s|
 
   s.add_runtime_dependency('gitlab', '>= 4.4.0', '< 5.0')
   s.add_runtime_dependency('httparty', '>= 0.13.7', '< 1.0')
-  s.add_runtime_dependency('octokit', '>= 4.7.0', '< 8.0')
+  s.add_runtime_dependency('octokit', '>= 4.7.0', '< 9.0')
   s.add_runtime_dependency('rainbow', '>= 2.2', '< 4.0')
   s.add_runtime_dependency('rexml', '>= 3.2.5', '< 4.0')
   s.add_runtime_dependency('rugged', '>= 0.23.0', '< 2.0')

spec/pronto/formatter/github_formatter_spec.rb

@@ -42,7 +42,7 @@ module Formatter
             Octokit::Client.any_instance
               .should_receive(:commit_comments)
               .once
-              .and_return([double(body: 'crucial', path: 'path/to', position: nil)])
+              .and_return([double(body: 'crucial', path: 'path/to', line: nil)])
 
             Octokit::Client.any_instance.should_not_receive(:create_commit_comment)
 
@@ -62,7 +62,7 @@ module Formatter
             Octokit::Client.any_instance
               .should_receive(:commit_comments)
               .once
-              .and_return([double(body: 'existed', path: 'path/to', position: nil)])
+              .and_return([double(body: 'existed', path: 'path/to', line: nil)])
 
             Octokit::Client.any_instance.should_receive(:create_commit_comment).once
 

spec/pronto/formatter/github_pull_request_formatter_spec.rb

@@ -12,28 +12,62 @@ module Formatter
         let(:patch) { repo.show_commit('64dadfd').first }
         let(:line) { patch.added_lines.first }
         let(:patches) { repo.diff('64dadfd^') }
+        let(:octokit_client) { instance_double(Octokit::Client) }
 
         before do
           ENV['PRONTO_PULL_REQUEST_ID'] = '10'
-          Octokit::Client.any_instance
-            .should_receive(:pull_requests)
+          Octokit::Client.stub(:new).and_return(octokit_client)
+          octokit_client
+            .stub(:pull_requests)
             .once
             .and_return([{ number: 10, head: { sha: 'foo' } }])
 
-          Octokit::Client.any_instance
-            .should_receive(:pull_comments)
+          octokit_client
+            .stub(:pull_comments)
             .once
-            .and_return([])
+            .and_return([double(body: 'a comment', path: 'a/path', line: 5)])
         end
 
         specify do
-          Octokit::Client.any_instance
+          octokit_client
             .should_receive(:create_pull_comment)
             .once
 
           subject
         end
 
+        context 'with duplicate comment' do
+          let(:messages) { [message] }
+          let(:message) { Message.new('path/to', line, :warning, 'existed') }
+          let(:line) { double(new_lineno: 3, commit_sha: '123', position: 3) }
+
+          specify do
+            octokit_client.should_receive(:pull_comments).and_return(
+              [double(body: 'existed', path: 'path/to', line: line.new_lineno)]
+            )
+
+            octokit_client.should_not_receive(:create_pull_comment)
+
+            subject.should eq '0 Pronto messages posted to GitHub (1 existing)'
+          end
+        end
+
+        context 'with one duplicate and one non duplicated comment' do
+          let(:messages) { [message, existing_message] }
+          let(:message) { Message.new('path/to', line, :warning, 'crucial') }
+          let(:existing_message) { Message.new('path/to', line, :warning, 'existed') }
+
+          specify do
+            octokit_client.should_receive(:pull_comments).and_return(
+              [double(body: 'existed', path: 'path/to', line: line.new_lineno)]
+            )
+
+            octokit_client.should_receive(:create_pull_comment).once
+
+            subject.should eq '1 Pronto messages posted to GitHub (1 existing)'
+          end
+        end
+
         context 'error handling' do
           let(:error_response) do
             {
@@ -54,7 +88,7 @@ module Formatter
 
           it 'handles and prints details' do
             error = Octokit::UnprocessableEntity.from_response(error_response)
-            Octokit::Client.any_instance
+            octokit_client
               .should_receive(:create_pull_comment)
               .and_raise(error)
 

spec/pronto/formatter/github_pull_request_review_formatter_spec.rb

@@ -12,23 +12,51 @@ module Formatter
         let(:patch) { repo.show_commit('64dadfd').first }
         let(:line) { patch.added_lines.first }
         let(:patches) { repo.diff('64dadfd^') }
+        let(:octokit_client) { instance_double(Octokit::Client) }
 
         before do
           ENV['PRONTO_PULL_REQUEST_ID'] = '10'
-
-          Octokit::Client.any_instance
-            .should_receive(:pull_comments)
-            .once
-            .and_return([])
+          Octokit::Client.stub(:new).and_return(octokit_client)
         end
 
         specify do
-          Octokit::Client.any_instance
-            .should_receive(:create_pull_request_review)
-            .once
+          octokit_client.should_receive(:pull_comments).and_return([])
+          octokit_client.should_receive(:create_pull_request_review).once
 
           subject
         end
+
+        context 'with duplicate comment' do
+          let(:messages) { [message] }
+          let(:message) { Message.new('path/to', line, :warning, 'existed') }
+          let(:line) { double(new_lineno: 3, commit_sha: '123', position: 3) }
+
+          specify do
+            octokit_client.should_receive(:pull_comments).and_return(
+              [double(body: 'existed', path: 'path/to', line: line.new_lineno)]
+            )
+
+            octokit_client.should_not_receive(:create_pull_request_review)
+
+            subject.should eq '0 Pronto messages posted to GitHub (1 existing)'
+          end
+        end
+
+        context 'with one duplicate and one non duplicated comment' do
+          let(:messages) { [message, existing_message] }
+          let(:message) { Message.new('path/to', line, :warning, 'crucial') }
+          let(:existing_message) { Message.new('path/to', line, :warning, 'existed') }
+
+          specify do
+            octokit_client.should_receive(:pull_comments).and_return(
+              [double(body: 'existed', path: 'path/to', line: line.new_lineno)]
+            )
+
+            octokit_client.should_receive(:create_pull_request_review).once
+
+            subject.should eq '1 Pronto messages posted to GitHub (1 existing)'
+          end
+        end
       end
     end
   end

spec/pronto/github_spec.rb

@@ -8,8 +8,7 @@ module Pronto
     let(:comment) { double(body: 'note', path: 'path', line: 1, position: 1) }
     let(:empty_client_options) do
       {
-        event: 'COMMENT',
-        accept: 'application/vnd.github.v3.diff+json'
+        event: 'COMMENT'
       }
     end
 
@@ -157,8 +156,8 @@ module Pronto
         end
         let(:options) do
           empty_client_options
-            .merge(comments: [{ path: 'bad_file.rb', position: 10, body: 'Offense #1' },
-                              { path: 'bad_file.rb', position: 20, body: 'Offense #2' }])
+            .merge(comments: [{ path: 'bad_file.rb', line: 10, body: 'Offense #1' },
+                              { path: 'bad_file.rb', line: 20, body: 'Offense #2' }])
         end
 
         {
@@ -184,11 +183,11 @@ module Pronto
           let(:warnings_per_review) { 1 }
           let(:first_options) do
             empty_client_options
-              .merge(comments: [{ path: 'bad_file.rb', position: 10, body: 'Offense #1' }])
+              .merge(comments: [{ path: 'bad_file.rb', line: 10, body: 'Offense #1' }])
           end
           let(:second_options) do
             empty_client_options
-              .merge(comments: [{ path: 'bad_file.rb', position: 20, body: 'Offense #2' }])
+              .merge(comments: [{ path: 'bad_file.rb', line: 20, body: 'Offense #2' }])
           end
 
           specify do