Repository: reviewdog/reviewdog
Pull request #675 (2020-07-21 01:39:51)

github-pr-review: Support line  based suggestions
- [x] Updated Unreleased section in [CHANGELOG](https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md) or it's not notable changes.




Patch:
diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml
index 91f9feaa..de63e70f 100644
--- a/.github/workflows/reviewdog.yml
+++ b/.github/workflows/reviewdog.yml
@@ -147,6 +147,12 @@ jobs:
           cd ./_testdata/ && reviewdog -conf=reviewdog_error.yml \
             -reporter=local -diff='git diff master' || EXIT_CODE=$?
           test "${EXIT_CODE}" = 1
+      - name: Suggestion
+        env:
+          REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          cat ./_testdata/suggestions.json | \
+            reviewdog -name="suggestion-test" -f=rdjsonl -reporter=github-pr-review
 
       - name: Custom rdjsonl test (github-pr-review)
         env:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a11ce40d..be91f3ff 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [#629](https://github.com/reviewdog/reviewdog/pull/629) Introduced Reviewdog Diagnostic Format. ([@haya14busa])
  - [#674](https://github.com/reviewdog/reviewdog/pull/674) Support rdjsonl as input format
  - [#680](https://github.com/reviewdog/reviewdog/pull/680) github-pr-review: Support multiline comments
+ - [#675](https://github.com/reviewdog/reviewdog/pull/675) github-pr-review: Support line based suggestions
 - ...
 
 ### :bug: Fixes
diff --git a/service/commentutil/commentutil.go b/service/commentutil/commentutil.go
index 1b78e8d0..3ece2638 100644
--- a/service/commentutil/commentutil.go
+++ b/service/commentutil/commentutil.go
@@ -13,7 +13,7 @@ type PostedComments map[string]map[int][]string
 // IsPosted returns true if a given comment has been posted in code review service already,
 // otherwise returns false. It sees comments with same path, same position,
 // and same body as same comments.
-func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int) bool {
+func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int, body string) bool {
 	path := c.Result.Diagnostic.GetLocation().GetPath()
 	if _, ok := p[path]; !ok {
 		return false
@@ -22,8 +22,8 @@ func (p PostedComments) IsPosted(c *reviewdog.Comment, lineNum int) bool {
 	if !ok {
 		return false
 	}
-	for _, body := range bodies {
-		if body == CommentBody(c) {
+	for _, b := range bodies {
+		if b == body {
 			return true
 		}
 	}
diff --git a/service/github/github.go b/service/github/github.go
index 38b457da..d2ae0a3a 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -2,6 +2,7 @@ package github
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"strings"
@@ -11,6 +12,7 @@ import (
 
 	"github.com/reviewdog/reviewdog"
 	"github.com/reviewdog/reviewdog/cienv"
+	"github.com/reviewdog/reviewdog/proto/rdf"
 	"github.com/reviewdog/reviewdog/service/commentutil"
 	"github.com/reviewdog/reviewdog/service/github/githubutils"
 	"github.com/reviewdog/reviewdog/service/serviceutil"
@@ -93,7 +95,8 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 			}
 			continue
 		}
-		if g.postedcs.IsPosted(c, githubCommentLine(c)) {
+		body := buildBody(c)
+		if g.postedcs.IsPosted(c, githubCommentLine(c), body) {
 			continue
 		}
 		// Only posts maxCommentsPerRequest comments per 1 request to avoid spammy
@@ -108,7 +111,7 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 			remaining = append(remaining, c)
 			continue
 		}
-		comments = append(comments, buildDraftReviewComment(c))
+		comments = append(comments, buildDraftReviewComment(c, body))
 	}
 
 	if len(comments) == 0 {
@@ -126,14 +129,13 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 }
 
 // Document: https://docs.github.com/en/rest/reference/pulls#create-a-review-comment-for-a-pull-request
-func buildDraftReviewComment(c *reviewdog.Comment) *github.DraftReviewComment {
-	cbody := commentutil.CommentBody(c)
+func buildDraftReviewComment(c *reviewdog.Comment, body string) *github.DraftReviewComment {
 	loc := c.Result.Diagnostic.GetLocation()
 	line := githubCommentLine(c)
 	r := &github.DraftReviewComment{
 		Path: github.String(loc.GetPath()),
 		Side: github.String("RIGHT"),
-		Body: github.String(cbody),
+		Body: github.String(body),
 		Line: github.Int(line),
 	}
 	// GitHub API: Start line must precede the end line.
@@ -250,3 +252,48 @@ func listAllPullRequestsComments(ctx context.Context, cli *github.Client,
 	}
 	return append(comments, restComments...), nil
 }
+
+func buildBody(c *reviewdog.Comment) string {
+	cbody := commentutil.CommentBody(c)
+	if suggestion := buildSuggestions(c); suggestion != "" {
+		cbody += "\n" + suggestion
+	}
+	return cbody
+}
+
+func buildSuggestions(c *reviewdog.Comment) string {
+	var sb strings.Builder
+	for _, s := range c.Result.Diagnostic.GetSuggestions() {
+		txt, err := buildSingleSuggestion(c, s)
+		if err != nil {
+			sb.WriteString(fmt.Sprintf("Invalid suggestion: %v", err))
+			continue
+		}
+		sb.WriteString(txt)
+		sb.WriteString("\n")
+	}
+	return sb.String()
+}
+
+func buildSingleSuggestion(c *reviewdog.Comment, s *rdf.Suggestion) (string, error) {
+	start := s.GetRange().GetStart()
+	end := s.GetRange().GetEnd()
+	drange := c.Result.Diagnostic.GetLocation().GetRange()
+	if start.GetLine() != drange.GetStart().GetLine() ||
+		end.GetLine() != drange.GetEnd().GetLine() {
+		return "", fmt.Errorf("the Diagnostic's lines and Suggestion lines must be the same. %d-%d v.s. %d-%d",
+			drange.GetStart().GetLine(), drange.GetEnd().GetLine(), start.GetLine(), end.GetLine())
+	}
+	if start.GetColumn() > 1 || end.GetColumn() > 1 {
+		// TODO(haya14busa): Support non-line based suggestion.
+		return "", errors.New("non line based suggestions (contains column) are not supported yet")
+	}
+	var sb strings.Builder
+	sb.WriteString("```suggestion\n")
+	if txt := s.GetText(); txt != "" {
+		sb.WriteString(txt)
+		sb.WriteString("\n")
+	}
+	sb.WriteString("```")
+	return sb.String(), nil
+}
diff --git a/service/gitlab/gitlab_mr_commit.go b/service/gitlab/gitlab_mr_commit.go
index bf4adf32..4686ab61 100644
--- a/service/gitlab/gitlab_mr_commit.go
+++ b/service/gitlab/gitlab_mr_commit.go
@@ -83,7 +83,8 @@ func (g *GitLabMergeRequestCommitCommenter) postCommentsForEach(ctx context.Cont
 		c := c
 		loc := c.Result.Diagnostic.GetLocation()
 		lnum := int(loc.GetRange().GetStart().GetLine())
-		if !c.Result.InDiffFile || lnum == 0 || g.postedcs.IsPosted(c, lnum) {
+		body := commentutil.CommentBody(c)
+		if !c.Result.InDiffFile || lnum == 0 || g.postedcs.IsPosted(c, lnum, body) {
 			continue
 		}
 		eg.Go(func() error {
@@ -91,7 +92,6 @@ func (g *GitLabMergeRequestCommitCommenter) postCommentsForEach(ctx context.Cont
 			if err != nil {
 				commitID = g.sha
 			}
-			body := commentutil.CommentBody(c)
 			prcomment := &gitlab.PostCommitCommentOptions{
 				Note:     gitlab.String(body),
 				Path:     gitlab.String(loc.GetPath()),
diff --git a/service/gitlab/gitlab_mr_discussion.go b/service/gitlab/gitlab_mr_discussion.go
index e96f50a0..c694111f 100644
--- a/service/gitlab/gitlab_mr_discussion.go
+++ b/service/gitlab/gitlab_mr_discussion.go
@@ -103,7 +103,8 @@ func (g *GitLabMergeRequestDiscussionCommenter) postCommentsForEach(ctx context.
 		c := c
 		loc := c.Result.Diagnostic.GetLocation()
 		lnum := int(loc.GetRange().GetStart().GetLine())
-		if !c.Result.InDiffFile || lnum == 0 || postedcs.IsPosted(c, lnum) {
+		body := commentutil.CommentBody(c)
+		if !c.Result.InDiffFile || lnum == 0 || postedcs.IsPosted(c, lnum, body) {
 			continue
 		}
 		eg.Go(func() error {
@@ -120,7 +121,7 @@ func (g *GitLabMergeRequestDiscussionCommenter) postCommentsForEach(ctx context.
 				pos.OldLine = c.Result.OldLine
 			}
 			discussion := &gitlab.CreateMergeRequestDiscussionOptions{
-				Body:     gitlab.String(commentutil.CommentBody(c)),
+				Body:     gitlab.String(body),
 				Position: pos,
 			}
 			_, _, err := g.cli.Discussions.CreateMergeRequestDiscussion(g.projects, g.pr, discussion)


Test patch:
diff --git a/_testdata/suggestions.go b/_testdata/suggestions.go
new file mode 100644
index 00000000..17316f2c
--- /dev/null
+++ b/_testdata/suggestions.go
@@ -0,0 +1,4 @@
+package testdata
+
+func suggestions() {
+}
diff --git a/_testdata/suggestions.json b/_testdata/suggestions.json
new file mode 100644
index 00000000..cbd23a47
--- /dev/null
+++ b/_testdata/suggestions.json
@@ -0,0 +1,2 @@
+{"message":"suggestion test","location":{"path":"_testdata/suggestions.go","range":{"start":{"line":3}}}, "suggestions": [{"range":{"start":{"line":3}},"text": "func Suggestions() {"}]}
+{"message":"multiline suggestion test","location":{"path":"_testdata/suggestions.go","range":{"start":{"line":3},"end":{"line":4}}}, "suggestions": [{"range":{"start":{"line":3},"end":{"line":4}}, "text": "func Suggestions() {\n// Multiline suggestion!\n} // end of suggestion func"}]}
diff --git a/service/github/github_test.go b/service/github/github_test.go
index 911aef68..d35be0dc 100644
--- a/service/github/github_test.go
+++ b/service/github/github_test.go
@@ -278,6 +278,55 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 				Line:      github.Int(16),
 				Body:      github.String(commentutil.BodyPrefix + "\nmultiline new comment"),
 			},
+			{
+				Path:      github.String("reviewdog.go"),
+				Side:      github.String("RIGHT"),
+				StartSide: github.String("RIGHT"),
+				StartLine: github.Int(15),
+				Line:      github.Int(16),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"multiline suggestion comment",
+					"```suggestion",
+					"line1",
+					"line2",
+					"line3",
+					"```",
+				}, "\n") + "\n"),
+			},
+			{
+				Path: github.String("reviewdog.go"),
+				Side: github.String("RIGHT"),
+				Line: github.Int(15),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"singleline suggestion comment",
+					"```suggestion",
+					"line1",
+					"line2",
+					"```",
+				}, "\n") + "\n"),
+			},
+			{
+				Path:      github.String("reviewdog.go"),
+				Side:      github.String("RIGHT"),
+				StartSide: github.String("RIGHT"),
+				StartLine: github.Int(15),
+				Line:      github.Int(16),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"invalid lines suggestion comment",
+					"Invalid suggestion: the Diagnostic's lines and Suggestion lines must be the same. 15-16 v.s. 16-17",
+				}, "\n")),
+			},
+			{
+				Path:      github.String("reviewdog.go"),
+				Side:      github.String("RIGHT"),
+				StartSide: github.String("RIGHT"),
+				StartLine: github.Int(15),
+				Line:      github.Int(16),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"non-line based suggestion comment",
+					"Invalid suggestion: non line based suggestions (contains column) are not supported yet",
+				}, "\n")),
+			},
 		}
 		if diff := pretty.Compare(want, req.Comments); diff != "" {
 			t.Errorf("req.Comments diff: (-got +want)\n%s", diff)
@@ -411,6 +460,130 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 			},
 			Body: "should not be reported via GitHub Review API",
 		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{
+								Line: 15,
+							},
+							End: &rdf.Position{
+								Line: 16,
+							},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+								End: &rdf.Position{
+									Line: 16,
+								},
+							},
+							Text: "line1\nline2\nline3",
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "multiline suggestion comment",
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{
+								Line: 15,
+							},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+							},
+							Text: "line1\nline2",
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "singleline suggestion comment",
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{
+								Line: 15,
+							},
+							End: &rdf.Position{
+								Line: 16,
+							},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 16,
+								},
+								End: &rdf.Position{
+									Line: 17,
+								},
+							},
+							Text: "line1\nline2\nline3",
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "invalid lines suggestion comment",
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{
+								Line: 15,
+							},
+							End: &rdf.Position{
+								Line: 16,
+							},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line:   15,
+									Column: 5,
+								},
+								End: &rdf.Position{
+									Line:   16,
+									Column: 7,
+								},
+							},
+							Text: "replacement",
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "non-line based suggestion comment",
+		},
 	}
 	for _, c := range comments {
 		if err := g.Post(context.Background(), c); err != nil {