Merge pull request #330 from reviewdog/remainig-as-summary

github-pr-review: use review summary comments for too many annotations

doghouse/server/doghouse.go

@@ -12,6 +12,7 @@ import (
 	"github.com/reviewdog/reviewdog"
 	"github.com/reviewdog/reviewdog/diff"
 	"github.com/reviewdog/reviewdog/doghouse"
+	"github.com/reviewdog/reviewdog/service/github/githubutils"
 )
 
 // GitHub check runs API cannot handle too large requests.
@@ -188,28 +189,13 @@ func (ch *Checker) summaryFindings(name string, checks []*reviewdog.FilteredChec
 			lines = append(lines, "... (Too many findings. Dropped some findings)")
 			break
 		}
-		lines = append(lines, ch.buildFindingLink(c))
+		lines = append(lines, githubutils.LinkedMarkdownCheckResult(
+			ch.req.Owner, ch.req.Repo, ch.req.SHA, c.CheckResult))
 	}
 	lines = append(lines, "</details>")
 	return lines
 }
 
-func (ch *Checker) buildFindingLink(c *reviewdog.FilteredCheck) string {
-	if c.Path == "" {
-		return c.Message
-	}
-	loc := c.Path
-	link := ch.brobHRef(c.Path)
-	if c.Lnum != 0 {
-		loc = fmt.Sprintf("%s:%d", loc, c.Lnum)
-		link = fmt.Sprintf("%s#L%d", link, c.Lnum)
-	}
-	if c.Col != 0 {
-		loc = fmt.Sprintf("%s:%d", loc, c.Col)
-	}
-	return fmt.Sprintf("[%s](%s): %s", loc, link, c.Message)
-}
-
 func (ch *Checker) toCheckRunAnnotation(c *reviewdog.FilteredCheck) *github.CheckRunAnnotation {
 	a := &github.CheckRunAnnotation{
 		Path:            github.String(c.Path),
@@ -227,10 +213,6 @@ func (ch *Checker) toCheckRunAnnotation(c *reviewdog.FilteredCheck) *github.Chec
 	return a
 }
 
-func (ch *Checker) brobHRef(path string) string {
-	return fmt.Sprintf("http://github.com/%s/%s/blob/%s/%s", ch.req.Owner, ch.req.Repo, ch.req.SHA, path)
-}
-
 func (ch *Checker) diff(ctx context.Context) ([]*diff.FileDiff, error) {
 	d, err := ch.rawDiff(ctx)
 	if err != nil {

service/github/github.go

@@ -4,16 +4,20 @@ import (
 	"context"
 	"fmt"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/google/go-github/v28/github"
 	"github.com/reviewdog/reviewdog"
+	"github.com/reviewdog/reviewdog/service/github/githubutils"
 	"github.com/reviewdog/reviewdog/service/serviceutil"
 )
 
 var _ reviewdog.CommentService = &GitHubPullRequest{}
 var _ reviewdog.DiffService = &GitHubPullRequest{}
 
+const maxCommentsPerRequest = 30
+
 // GitHubPullRequest is a comment and diff service for GitHub PullRequest.
 //
 // API:
@@ -75,10 +79,23 @@ func (g *GitHubPullRequest) Flush(ctx context.Context) error {
 
 func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 	comments := make([]*github.DraftReviewComment, 0, len(g.postComments))
+	remaining := make([]*reviewdog.Comment, 0)
 	for _, c := range g.postComments {
 		if g.postedcs.IsPosted(c, c.LnumDiff) {
 			continue
 		}
+		// Only posts maxCommentsPerRequest comments per 1 request to avoid spammy
+		// review comments. An example GitHub error if we don't limit the # of
+		// review comments.
+		//
+		// > 403 You have triggered an abuse detection mechanism and have been
+		// > temporarily blocked from content creation. Please retry your request
+		// > again later.
+		// https://developer.github.com/v3/#abuse-rate-limits
+		if len(comments) >= maxCommentsPerRequest {
+			remaining = append(remaining, c)
+			continue
+		}
 		cbody := serviceutil.CommentBody(c)
 		comments = append(comments, &github.DraftReviewComment{
 			Path:     &c.Path,
@@ -97,11 +114,36 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 		CommitID: &g.sha,
 		Event:    github.String("COMMENT"),
 		Comments: comments,
+		Body:     github.String(g.remainingCommentsSummary(remaining)),
 	}
 	_, _, err := g.cli.PullRequests.CreateReview(ctx, g.owner, g.repo, g.pr, review)
 	return err
 }
 
+func (g *GitHubPullRequest) remainingCommentsSummary(remaining []*reviewdog.Comment) string {
+	if len(remaining) == 0 {
+		return ""
+	}
+	perTool := make(map[string][]*reviewdog.Comment)
+	for _, c := range remaining {
+		perTool[c.ToolName] = append(perTool[c.ToolName], c)
+	}
+	var sb strings.Builder
+	sb.WriteString("Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit\n")
+	sb.WriteString("\n")
+	for tool, comments := range perTool {
+		sb.WriteString("<details>\n")
+		sb.WriteString(fmt.Sprintf("<summary>%s</summary>\n", tool))
+		sb.WriteString("\n")
+		for _, c := range comments {
+			sb.WriteString(githubutils.LinkedMarkdownCheckResult(g.owner, g.repo, g.sha, c.CheckResult))
+			sb.WriteString("\n")
+		}
+		sb.WriteString("</details>\n")
+	}
+	return sb.String()
+}
+
 func (g *GitHubPullRequest) setPostedComment(ctx context.Context) error {
 	g.postedcs = make(serviceutil.PostedComments)
 	cs, err := g.comment(ctx)

service/github/github_test.go

@@ -221,7 +221,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 		if *req.Event != "COMMENT" {
 			t.Errorf("PullRequestReviewRequest.Event = %v, want COMMENT", *req.Event)
 		}
-		if req.Body != nil {
+		if req.Body != nil && *req.Body != "" {
 			t.Errorf("PullRequestReviewRequest.Body = %v, want empty", *req.Body)
 		}
 		if *req.CommitID != "sha" {
@@ -286,6 +286,69 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 	}
 }
 
+func TestGitHubPullRequest_Post_toomany(t *testing.T) {
+	cwd, _ := os.Getwd()
+	defer os.Chdir(cwd)
+
+	moveToRootDir()
+
+	listCommentsAPICalled := 0
+	postCommentsAPICalled := 0
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/repos/o/r/pulls/14/comments", func(w http.ResponseWriter, r *http.Request) {
+		listCommentsAPICalled++
+		if err := json.NewEncoder(w).Encode([]*github.PullRequestComment{}); err != nil {
+			t.Fatal(err)
+		}
+	})
+	mux.HandleFunc("/repos/o/r/pulls/14/reviews", func(w http.ResponseWriter, r *http.Request) {
+		postCommentsAPICalled++
+		var req github.PullRequestReviewRequest
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+			t.Error(err)
+		}
+		if req.GetBody() == "" {
+			t.Errorf("PullRequestReviewRequest.Body is empty but want some summary text")
+		}
+	})
+	ts := httptest.NewServer(mux)
+	defer ts.Close()
+
+	cli := github.NewClient(nil)
+	cli.BaseURL, _ = url.Parse(ts.URL + "/")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha")
+	if err != nil {
+		t.Fatal(err)
+	}
+	comments := []*reviewdog.Comment{}
+	for i := 0; i < 100; i++ {
+		comments = append(comments, &reviewdog.Comment{
+			CheckResult: &reviewdog.CheckResult{
+				Path: "reviewdog.go",
+				Lnum: i,
+			},
+			LnumDiff: i,
+			Body:     "comment",
+			ToolName: "tool",
+		})
+	}
+	for _, c := range comments {
+		if err := g.Post(context.Background(), c); err != nil {
+			t.Error(err)
+		}
+	}
+	if err := g.Flush(context.Background()); err != nil {
+		t.Error(err)
+	}
+	if want := 1; listCommentsAPICalled != want {
+		t.Errorf("GitHub List PullRequest comments API called %v times, want %d times", listCommentsAPICalled, want)
+	}
+	if want := 1; postCommentsAPICalled != want {
+		t.Errorf("GitHub post PullRequest comments API called %v times, want %d times", postCommentsAPICalled, want)
+	}
+}
+
 func TestGitHubPullRequest_workdir(t *testing.T) {
 	cwd, _ := os.Getwd()
 	defer os.Chdir(cwd)

service/github/githubutils/utils.go

@@ -0,0 +1,43 @@
+package githubutils
+
+import (
+	"fmt"
+
+	"github.com/reviewdog/reviewdog"
+)
+
+// LinkedMarkdownCheckResult returns Markdown string which contains a link to the
+// location in the CheckResult and CheckResult content itself.
+func LinkedMarkdownCheckResult(owner, repo, sha string, c *reviewdog.CheckResult) string {
+	if c.Path == "" {
+		return c.Message
+	}
+	loc := BasicLocationFormat(c)
+	link := PathLink(owner, repo, sha, c.Path, c.Lnum)
+	return fmt.Sprintf("[%s](%s) %s", loc, link, c.Message)
+}
+
+// PathLink build a link to GitHub path to given sha, file, and line.
+func PathLink(owner, repo, sha, path string, line int) string {
+	if sha == "" {
+		sha = "master"
+	}
+	fragment := ""
+	if line > 0 {
+		fragment = fmt.Sprintf("#L%d", line)
+	}
+	return fmt.Sprintf("http://github.com/%s/%s/blob/%s/%s%s",
+		owner, repo, sha, path, fragment)
+}
+
+// BasicLocationFormat format chec CheckResult to %f|%l col %c| erorformat.
+func BasicLocationFormat(c *reviewdog.CheckResult) string {
+	loc := c.Path + "|"
+	if c.Lnum != 0 {
+		loc = fmt.Sprintf("%s%d", loc, c.Lnum)
+		if c.Col != 0 {
+			loc = fmt.Sprintf("%s col %d", loc, c.Col)
+		}
+	}
+	return loc + "|"
+}

service/github/githubutils/utils_test.go

@@ -0,0 +1,58 @@
+package githubutils
+
+import (
+	"testing"
+
+	"github.com/reviewdog/reviewdog"
+)
+
+func TestLinkedMarkdownCheckResult(t *testing.T) {
+	tests := []struct {
+		owner, repo, sha string
+		c                *reviewdog.CheckResult
+		want             string
+	}{
+		{
+			owner: "o",
+			repo:  "r",
+			sha:   "s",
+			c: &reviewdog.CheckResult{
+				Path:    "path/to/file.txt",
+				Lnum:    1414,
+				Col:     14,
+				Message: "msg",
+			},
+			want: "[path/to/file.txt|1414 col 14|](http://github.com/o/r/blob/s/path/to/file.txt#L1414) msg",
+		},
+		{
+			owner: "o",
+			repo:  "r",
+			sha:   "s",
+			c: &reviewdog.CheckResult{
+				Path:    "path/to/file.txt",
+				Lnum:    1414,
+				Col:     0,
+				Message: "msg",
+			},
+			want: "[path/to/file.txt|1414|](http://github.com/o/r/blob/s/path/to/file.txt#L1414) msg",
+		},
+		{
+			owner: "o",
+			repo:  "r",
+			sha:   "s",
+			c: &reviewdog.CheckResult{
+				Path:    "path/to/file.txt",
+				Lnum:    0,
+				Col:     0,
+				Message: "msg",
+			},
+			want: "[path/to/file.txt||](http://github.com/o/r/blob/s/path/to/file.txt) msg",
+		},
+	}
+	for _, tt := range tests {
+		if got := LinkedMarkdownCheckResult(tt.owner, tt.repo, tt.sha, tt.c); got != tt.want {
+			t.Errorf("LinkedMarkdownCheckResult(%q, %q, %q, %#v) = %q, want %q",
+				tt.owner, tt.repo, tt.sha, tt.c, got, tt.want)
+		}
+	}
+}