diff --git a/lib/pronto/formatter/github_pull_request_formatter.rb b/lib/pronto/formatter/github_pull_request_formatter.rb index 93858845..e0778b10 100644 --- a/lib/pronto/formatter/github_pull_request_formatter.rb +++ b/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 diff --git a/lib/pronto/formatter/github_pull_request_review_formatter.rb b/lib/pronto/formatter/github_pull_request_review_formatter.rb index f510ccf5..2183c701 100644 --- a/lib/pronto/formatter/github_pull_request_review_formatter.rb +++ b/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 diff --git a/lib/pronto/github.rb b/lib/pronto/github.rb index 72b6cef2..dc702cea 100644 --- a/lib/pronto/github.rb +++ b/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 } diff --git a/pronto.gemspec b/pronto.gemspec index 44e39161..55257485 100644 --- a/pronto.gemspec +++ b/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') diff --git a/spec/pronto/formatter/github_formatter_spec.rb b/spec/pronto/formatter/github_formatter_spec.rb index 1fdcfc8a..2d4322ee 100644 --- a/spec/pronto/formatter/github_formatter_spec.rb +++ b/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 diff --git a/spec/pronto/formatter/github_pull_request_formatter_spec.rb b/spec/pronto/formatter/github_pull_request_formatter_spec.rb index 61e9fc30..bf2689ac 100644 --- a/spec/pronto/formatter/github_pull_request_formatter_spec.rb +++ b/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) diff --git a/spec/pronto/formatter/github_pull_request_review_formatter_spec.rb b/spec/pronto/formatter/github_pull_request_review_formatter_spec.rb index 4ece5fbb..c01a37ef 100644 --- a/spec/pronto/formatter/github_pull_request_review_formatter_spec.rb +++ b/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 diff --git a/spec/pronto/github_spec.rb b/spec/pronto/github_spec.rb index d047eb25..0991c7c7 100644 --- a/spec/pronto/github_spec.rb +++ b/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