Feature/f2018071301 add option for warnings per pr review

README.md

@@ -116,6 +116,16 @@ If you want review to appear on pull request diff, instead of comments:
 $ PRONTO_GITHUB_ACCESS_TOKEN=token pronto run -f github_pr_review -c origin/master
 ```
 
+All the comments will now not be published into a single PR review, but separated into a number of PRs.
+This can be managed via additional environment variable or in the config file (see below):
+
+Note: In case no environment variable or config setting is specified in `.pronto.yml`, 
+      a default value of `30` will be used.
+
+```sh
+$ PRONTO_WARNINGS_PER_REVIEW=30 PRONTO_GITHUB_ACCESS_TOKEN=token pronto run -f github_pr_review -c origin/master
+```
+
 Use `GithubStatusFormatter` to submit [commit status](https://github.com/blog/1227-commit-status-api):
 
 ```sh
@@ -210,6 +220,7 @@ bitbucket:
   password: pass
   web_endpoint: https://bitbucket.org/
 max_warnings: 150
+warnings_per_review: 30
 verbose: false
 ```
 

lib/pronto/config.rb

@@ -39,6 +39,10 @@ def bitbucket_hostname
       URI.parse(bitbucket_web_endpoint).host
     end
 
+    def warnings_per_review
+      ENV['PRONTO_WARNINGS_PER_REVIEW'] && Integer(ENV['PRONTO_WARNINGS_PER_REVIEW']) || @config_hash['warnings_per_review']
+    end
+
     def max_warnings
       ENV['PRONTO_MAX_WARNINGS'] && Integer(ENV['PRONTO_MAX_WARNINGS']) || @config_hash['max_warnings']
     end

lib/pronto/config_file.rb

@@ -1,6 +1,7 @@
 module Pronto
   class ConfigFile
     DEFAULT_MESSAGE_FORMAT = '%{msg}'.freeze
+    DEFAULT_WARNINGS_PER_REVIEW = 30
 
     EMPTY = {
       'all' => {
@@ -32,6 +33,7 @@ class ConfigFile
       'runners' => [],
       'formatters' => [],
       'max_warnings' => nil,
+      'warnings_per_review' => DEFAULT_WARNINGS_PER_REVIEW,
       'verbose' => false,
       'format' => DEFAULT_MESSAGE_FORMAT
     }.freeze

lib/pronto/formatter/github_pull_request_review_formatter.rb

@@ -10,7 +10,7 @@ def pretty_name
       end
 
       def submit_comments(client, comments)
-        client.create_pull_request_review(comments)
+        client.publish_pull_request_comments(comments)
       rescue Octokit::UnprocessableEntity, HTTParty::Error => e
         $stderr.puts "Failed to post: #{e.message}"
       end

lib/pronto/github.rb

@@ -45,17 +45,12 @@ def create_pull_comment(comment)
       end
     end
 
-    def create_pull_request_review(comments)
-      return if comments.empty?
-
-      options = {
-        event: 'COMMENT',
-        accept: 'application/vnd.github.v3.diff+json', # https://developer.github.com/v3/pulls/reviews/#create-a-pull-request-review
-        comments: comments.map do |c|
-          { path: c.path, position: c.position, body: c.body }
-        end
-      }
-      client.create_pull_request_review(slug, pull_id, options)
+    def publish_pull_request_comments(comments)
+      comments_left = comments.clone
+      while comments_left.any?
+        comments_to_publish = comments_left.slice!(0, warnings_per_review)
+        create_pull_request_review(comments_to_publish)
+      end
     end
 
     def create_commit_status(status)
@@ -68,6 +63,21 @@ def create_commit_status(status)
 
     private
 
+    def create_pull_request_review(comments)
+      options = {
+        event: 'COMMENT',
+        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
+          }
+        end
+      }
+      client.create_pull_request_review(slug, pull_id, options)
+    end
+
     def slug
       return @config.github_slug if @config.github_slug
       @slug ||= begin
@@ -103,5 +113,9 @@ def pull
                   @github_pull.pull_by_commit(@repo.head_commit_sha)
                 end
     end
+
+    def warnings_per_review
+      @warnings_per_review ||= @config.warnings_per_review
+    end
   end
 end

spec/pronto/config_spec.rb

@@ -56,32 +56,41 @@ module Pronto
       end
     end
 
-    describe '#max_warnings' do
-      subject { config.max_warnings }
-
-      context 'from env variable' do
-        context 'with a valid value' do
-          before { stub_const('ENV', 'PRONTO_MAX_WARNINGS' => '20') }
-          it { should == 20 }
-        end
+    {
+      max_warnings: {
+        default_value: nil
+      },
+      warnings_per_review: {
+        default_value: ConfigFile::DEFAULT_WARNINGS_PER_REVIEW
+      }
+    }.each do |setting_name, specifics|
+      describe "##{setting_name}" do
+        subject { config.public_send(setting_name) }
+
+        context 'from env variable' do
+          context 'with a valid value' do
+            before { stub_const('ENV', "PRONTO_#{setting_name.upcase}" => '20') }
+            it { should == 20 }
+          end
 
-        context 'with an invalid value' do
-          before { stub_const('ENV', 'PRONTO_MAX_WARNINGS' => 'twenty') }
+          context 'with an invalid value' do
+            before { stub_const('ENV', "PRONTO_#{setting_name.upcase}" => 'twenty') }
 
-          specify do
-            -> { subject }.should raise_error(ArgumentError)
+            specify do
+              -> { subject }.should raise_error(ArgumentError)
+            end
           end
         end
-      end
 
-      context 'from config hash' do
-        let(:config_hash) { { 'max_warnings' => 40 } }
-        it { should == 40 }
-      end
+        context 'from config hash' do
+          let(:config_hash) { { setting_name.to_s => 40 } }
+          it { should == 40 }
+        end
 
-      context 'default' do
-        let(:config_hash) { ConfigFile::EMPTY }
-        it { should == nil }
+        context 'default' do
+          let(:config_hash) { ConfigFile::EMPTY }
+          it { should == specifics[:default_value] }
+        end
       end
     end
 

spec/pronto/formatter/github_formatter_spec.rb

@@ -5,50 +5,54 @@ module Formatter
 
       describe '#format' do
         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') }
-        let(:line) { double(new_lineno: 1, commit_sha: '123', position: nil) }
-        before { line.stub(:commit_line).and_return(line) }
-
-        specify do
-          Octokit::Client.any_instance
-            .should_receive(:commit_comments)
-            .once
-            .and_return([])
-
-          Octokit::Client.any_instance
-            .should_receive(:create_commit_comment)
-            .once
 
-          subject
-        end
-      end
-
-      describe '#format without duplicates' do
-        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') }
-        let(:message2) { Message.new('path/to2', line2, :warning, 'crucial') }
-        let(:line1) { double(new_lineno: 1, commit_sha: '123', position: nil) }
-        let(:line2) { double(new_lineno: 2, commit_sha: '123', position: nil) }
-        before do
-          line1.stub(:commit_line).and_return(line1)
-          line2.stub(:commit_line).and_return(line2)
+        let(:published_comments_msg) { "%{count} Pronto messages posted to #{formatter.pretty_name}" }
+
+        context 'with duplicates' do
+          let(:messages) { [message, message] }
+          let(:message) { Message.new('path/to', line, :warning, 'crucial') }
+          let(:line) { double(new_lineno: 1, commit_sha: '123', position: nil) }
+          before { line.stub(:commit_line).and_return(line) }
+
+          specify do
+            Octokit::Client.any_instance
+              .should_receive(:commit_comments)
+              .once
+              .and_return([])
+
+            Octokit::Client.any_instance
+              .should_receive(:create_commit_comment)
+              .once
+
+            subject.should eq format(published_comments_msg, count: 1)
+          end
         end
 
-        specify do
-          Octokit::Client.any_instance
-            .should_receive(:commit_comments)
-            .once
-            .and_return([])
-
-          Octokit::Client.any_instance
-            .should_receive(:create_commit_comment)
-            .twice
-
-          subject
+        context 'without duplicates' do
+          let(:messages) { [message1, message2] }
+          let(:message1) { Message.new('path/to1', line1, :warning, 'crucial') }
+          let(:message2) { Message.new('path/to2', line2, :warning, 'crucial') }
+          let(:line1) { double(new_lineno: 1, commit_sha: '123', position: nil) }
+          let(:line2) { double(new_lineno: 2, commit_sha: '123', position: nil) }
+
+          before do
+            line1.stub(:commit_line).and_return(line1)
+            line2.stub(:commit_line).and_return(line2)
+          end
+
+          specify do
+            Octokit::Client.any_instance
+              .should_receive(:commit_comments)
+              .once
+              .and_return([])
+
+            Octokit::Client.any_instance
+              .should_receive(:create_commit_comment)
+              .twice
+
+            subject.should eq format(published_comments_msg, count: 2)
+          end
         end
       end
     end