Merge pull request #80 from xystushi/fix_comments_count

[Review] Request from 'xystushi' @ 'xystushi/git-review/fix_comments_count'

lib/git-review/commands.rb

@@ -12,14 +12,13 @@ def list(reverse=false)
         # explicitly look for local changes Github does not yet know about
         local.merged?(request.head.sha)
       }
-      requests.reverse! if reverse
       source = local.source
       if requests.empty?
         puts "No pending requests for '#{source}'."
       else
         puts "Pending requests for '#{source}':"
         puts "ID      Updated    Comments  Title"
-        requests.each { |request| print_request(request) }
+        print_requests(requests, reverse)
       end
     end
 
@@ -173,18 +172,31 @@ def console
 
   private
 
-    def print_request(request)
+    def request_summary_line(request)
       date_string = format_time(request.updated_at)
-      comments_count = request.comments.to_i + request.review_comments.to_i
+      comments_count = github.comments_count(request)
       line = format_text(request.number, 8)
       line << format_text(date_string, 11)
       line << format_text(comments_count, 10)
       line << format_text(request.title, 91)
-      puts line
+      line
+    end
+
+    def print_requests(requests, reverse=false)
+      # put all output lines in a hash first, keyed by request number
+      # this is to make sure the order is still correct even if we use
+      #   multi-threading to retrieve the requests
+      output = {}
+      requests.each { |req| output[req.number] = request_summary_line(req) }
+      numbers = output.keys.sort
+      numbers.reverse! if reverse
+      numbers.each do |n|
+        puts output[n]
+      end
     end
 
     def print_request_details(request)
-      comments_count = request.comments.to_i + request.review_comments.to_i
+      comments_count = github.comments_count(request)
       puts 'ID        : ' + request.number.to_s
       puts 'Label     : ' + request.head.label
       puts 'Updated   : ' + format_time(request.updated_at)

lib/git-review/github.rb

@@ -71,9 +71,15 @@ def current_requests(repo=source_repo)
 
     # a more detailed collection of requests
     def current_requests_full(repo=source_repo)
-      @github.pull_requests(repo).collect { |request|
-        @github.pull_request(repo, request.number)
-      }
+      threads = []
+      requests = []
+      @github.pull_requests(repo).each do |req|
+        threads << Thread.new {
+          requests << @github.pull_request(repo, req.number)
+        }
+      end
+      threads.each { |t| t.join }
+      requests
     end
 
     def update
@@ -144,7 +150,8 @@ def commit_discussion(number)
     end
 
     def issue_discussion(number)
-      comments = @github.issue_comments(source_repo, number)
+      comments = @github.issue_comments(source_repo, number) +
+          @github.review_comments(source_repo, number)
       discussion = ["\nComments on pull request:\n\n"]
       discussion += comments.collect { |comment|
         name = comment.user.login
@@ -161,6 +168,14 @@ def issue_discussion(number)
       discussion.compact.flatten unless discussion.empty?
     end
 
+    # get the number of comments, including comments on commits
+    def comments_count(request)
+      issue_c = request.comments + request.review_comments
+      commits_c = @github.pull_commits(source_repo, request.number).
+          inject(0) { |sum, c| sum + c.commit.comment_count }
+      issue_c + commits_c
+    end
+
     # show discussion for a request
     def discussion(number)
       commit_discussion(number) +

spec/git-review/commands_spec.rb

@@ -33,15 +33,13 @@
       it 'shows them' do
         subject.should_receive(:puts).with(/Pending requests for 'some_source'/)
         subject.should_not_receive(:puts).with(/No pending requests/)
-        subject.should_receive(:print_request).with(req1).ordered
-        subject.should_receive(:print_request).with(req2).ordered
+        subject.should_receive(:print_requests).with([req1, req2], false)
         subject.list
       end
 
       it 'sorts the output with --reverse option' do
         subject.stub(:puts)
-        subject.should_receive(:print_request).with(req2).ordered
-        subject.should_receive(:print_request).with(req1).ordered
+        subject.should_receive(:print_requests).with([req1, req2], true)
         subject.list(true)
       end