Repository: reviewdog/reviewdog
Pull request #680 (2020-07-22 06:56:00)

github-pr-review: Support multiline comments
- [ ] 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 6bd90fd6..d32764f1 100644
--- a/.github/workflows/reviewdog.yml
+++ b/.github/workflows/reviewdog.yml
@@ -149,6 +149,13 @@ jobs:
             -reporter=local -diff='git diff master' || EXIT_CODE=$?
           test "${EXIT_CODE}" = 1
 
+      - name: Multiline
+        env:
+          REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          cat ./_testdata/custom_rdjsonl.json | \
+            reviewdog -name="multiline-test" -f=rdjsonl -reporter=github-pr-review
+
   golangci-lint:
     if: github.event_name == 'pull_request'
     name: runner / golangci-lint
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 059bd385..a11ce40d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### :rocket: Enhancements
 - [#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
 - ...
 
 ### :bug: Fixes
diff --git a/difffilter/filter.go b/difffilter/filter.go
index 2bc4ee2e..e33cc4bf 100644
--- a/difffilter/filter.go
+++ b/difffilter/filter.go
@@ -113,7 +113,7 @@ func (df *DiffFilter) addDiff(filediffs []*diff.FileDiff) {
 		}
 		for _, hunk := range filediff.Hunks {
 			for _, line := range hunk.Lines {
-				if line.LnumNew > 0 && df.isSignificantLine(line) {
+				if line.LnumNew > 0 {
 					lines[line.LnumNew] = line
 				}
 			}
@@ -135,7 +135,7 @@ func (df *DiffFilter) ShouldReport(path string, lnum int) (bool, *diff.FileDiff,
 	if !ok {
 		return (df.mode == ModeNoFilter || df.mode == ModeFile), file, nil
 	}
-	return true, file, line
+	return df.isSignificantLine(line), file, line
 }
 
 func (df *DiffFilter) isSignificantLine(line *diff.Line) bool {
diff --git a/doghouse/appengine/app.yaml b/doghouse/appengine/app.yaml
index ccb64d3f..f7f33fd2 100644
--- a/doghouse/appengine/app.yaml
+++ b/doghouse/appengine/app.yaml
@@ -1,7 +1,7 @@
 # reviewdog app config: https://github.com/settings/apps/reviewdog
 
 # https://cloud.google.com/appengine/docs/standard/go/config/appref
-runtime: go114
+runtime: go112
 
 # https://cloud.google.com/appengine/docs/standard/go/warmup-requests/configuring
 inbound_services:
diff --git a/filter.go b/filter.go
index 94fe3994..09649c28 100644
--- a/filter.go
+++ b/filter.go
@@ -11,10 +11,14 @@ import (
 type FilteredCheck struct {
 	*CheckResult
 	ShouldReport bool
-	LnumDiff     int  // 0 if the result is outside diff.
-	InDiffFile   bool // false if the result is outside diff files.
-	OldPath      string
-	OldLine      int
+	// false if the result is outside diff files.
+	InDiffFile bool
+	// true if the result is inside a diff hunk.
+	// If it's a multiline result, both start and end must be in the same diff
+	// hunk.
+	InDiffContext bool
+	OldPath       string
+	OldLine       int
 }
 
 // FilterCheck filters check results by diff. It doesn't drop check which
@@ -26,17 +30,26 @@ func FilterCheck(results []*CheckResult, diff []*diff.FileDiff, strip int,
 	for _, result := range results {
 		check := &FilteredCheck{CheckResult: result}
 		loc := result.Diagnostic.GetLocation()
-		lnum := int(loc.GetRange().GetStart().GetLine())
-		shouldReport, difffile, diffline := df.ShouldReport(loc.GetPath(), lnum)
-		check.ShouldReport = shouldReport
-		if diffline != nil {
-			check.LnumDiff = diffline.LnumDiff
+		startLine := int(loc.GetRange().GetStart().GetLine())
+		endLine := int(loc.GetRange().GetEnd().GetLine())
+		if endLine == 0 {
+			endLine = startLine
 		}
-		loc.Path = CleanPath(loc.GetPath(), cwd)
-		if difffile != nil {
-			check.InDiffFile = true
-			check.OldPath, check.OldLine = getOldPosition(difffile, strip, loc.GetPath(), lnum)
+		check.InDiffContext = true
+		for l := startLine; l <= endLine; l++ {
+			shouldReport, difffile, diffline := df.ShouldReport(loc.GetPath(), l)
+			check.ShouldReport = check.ShouldReport || shouldReport
+			// all lines must be in diff.
+			check.InDiffContext = check.InDiffContext && diffline != nil
+			if difffile != nil {
+				check.InDiffFile = true
+				if l == startLine {
+					// TODO(haya14busa): Support endline as well especially for GitLab.
+					check.OldPath, check.OldLine = getOldPosition(difffile, strip, loc.GetPath(), l)
+				}
+			}
 		}
+		loc.Path = CleanPath(loc.GetPath(), cwd)
 		checks = append(checks, check)
 	}
 	return checks
@@ -44,6 +57,9 @@ func FilterCheck(results []*CheckResult, diff []*diff.FileDiff, strip int,
 
 // CleanPath clean up given path. If workdir is not empty, it returns relative
 // path to the given workdir.
+//
+// TODO(haya14busa): DRY. Create shared logic between this and
+// difffilter.normalizePath.
 func CleanPath(path, workdir string) string {
 	p := path
 	if filepath.IsAbs(path) && workdir != "" {
diff --git a/service/github/github.go b/service/github/github.go
index 8e00e58a..15fceca6 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -85,7 +85,7 @@ 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 c.Result.LnumDiff == 0 {
+		if !c.Result.InDiffContext {
 			// GitHub Review API cannot report results outside diff. If it's running
 			// in GitHub Actions, fallback to GitHub Actions log as report .
 			if cienv.IsInGitHubAction() {
@@ -93,7 +93,7 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 			}
 			continue
 		}
-		if g.postedcs.IsPosted(c, c.Result.LnumDiff) {
+		if g.postedcs.IsPosted(c, githubCommentLine(c)) {
 			continue
 		}
 		// Only posts maxCommentsPerRequest comments per 1 request to avoid spammy
@@ -108,12 +108,7 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 			remaining = append(remaining, c)
 			continue
 		}
-		cbody := commentutil.CommentBody(c)
-		comments = append(comments, &github.DraftReviewComment{
-			Path:     github.String(c.Result.Diagnostic.GetLocation().GetPath()),
-			Position: github.Int(c.Result.LnumDiff),
-			Body:     github.String(cbody),
-		})
+		comments = append(comments, buildDraftReviewComment(c))
 	}
 
 	if len(comments) == 0 {
@@ -130,6 +125,42 @@ func (g *GitHubPullRequest) postAsReviewComment(ctx context.Context) error {
 	return err
 }
 
+// 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)
+	loc := c.Result.Diagnostic.GetLocation()
+	line := githubCommentLine(c)
+	r := &github.DraftReviewComment{
+		Path: github.String(loc.GetPath()),
+		Side: github.String("RIGHT"),
+		Body: github.String(cbody),
+		Line: github.Int(line),
+	}
+	// GitHub API: Start line must precede the end line.
+	if startLine := int(loc.GetRange().GetStart().GetLine()); startLine < line {
+		r.StartSide = github.String("RIGHT")
+		r.StartLine = github.Int(startLine)
+	}
+	return r
+}
+
+// line represents end line if it's a multiline comment in GitHub, otherwise
+// it's start line.
+// Document: https://docs.github.com/en/rest/reference/pulls#create-a-review-comment-for-a-pull-request
+func githubCommentLine(c *reviewdog.Comment) int {
+	loc := c.Result.Diagnostic.GetLocation()
+	line := loc.GetRange().GetEnd().GetLine()
+	// End position with column == 1 means range to the end of the previous lines
+	// including line-break.
+	if loc.GetRange().GetEnd().GetColumn() == 1 {
+		line--
+	}
+	if line == 0 {
+		line = loc.GetRange().GetStart().GetLine()
+	}
+	return int(line)
+}
+
 func (g *GitHubPullRequest) remainingCommentsSummary(remaining []*reviewdog.Comment) string {
 	if len(remaining) == 0 {
 		return ""
@@ -161,12 +192,10 @@ func (g *GitHubPullRequest) setPostedComment(ctx context.Context) error {
 		return err
 	}
 	for _, c := range cs {
-		if c.Position == nil || c.Path == nil || c.Body == nil {
-			// skip resolved comments. Or comments which do not have "path" nor
-			// "body".
+		if c.Line == nil || c.Path == nil || c.Body == nil {
 			continue
 		}
-		g.postedcs.AddPostedComment(c.GetPath(), c.GetPosition(), c.GetBody())
+		g.postedcs.AddPostedComment(c.GetPath(), c.GetLine(), c.GetBody())
 	}
 	return nil
 }


Test patch:
diff --git a/_testdata/custom.txt b/_testdata/custom.txt
new file mode 100644
index 00000000..9bda1ee0
--- /dev/null
+++ b/_testdata/custom.txt
@@ -0,0 +1,9 @@
+Custom text for testing
+
+line 5
+
+--- Range
+	a = 1
+	b = 2
+	c = 3
+---
diff --git a/_testdata/custom_rdjsonl.json b/_testdata/custom_rdjsonl.json
new file mode 100644
index 00000000..70ab8477
--- /dev/null
+++ b/_testdata/custom_rdjsonl.json
@@ -0,0 +1,4 @@
+{"message":"multiline test 1","location":{"path":"_testdata/custom.txt","range":{"start":{"line":5}, "end":{"line":9}}}}
+{"message":"multiline test 2 (same line)","location":{"path":"_testdata/custom.txt","range":{"start":{"line":5}, "end":{"line":5}}}}
+{"message":"multiline test 3 (with line-break)","location":{"path":"_testdata/custom.txt","range":{"start":{"line":6,"column":2}, "end":{"line":7,"column":1}}}}
+{"message":"multiline test 4 (with line-break)","location":{"path":"_testdata/custom.txt","range":{"start":{"line":6,"column":2}, "end":{"line":9,"column":1}}}}
diff --git a/cmd/reviewdog/doghouse_test.go b/cmd/reviewdog/doghouse_test.go
index 37acf666..8dcae649 100644
--- a/cmd/reviewdog/doghouse_test.go
+++ b/cmd/reviewdog/doghouse_test.go
@@ -321,7 +321,7 @@ func TestPostResultSet_withoutReportURL(t *testing.T) {
 		sha   = "1414"
 	)
 
-	wantResults := []*reviewdog.FilteredCheck{{LnumDiff: 1}}
+	wantResults := []*reviewdog.FilteredCheck{{ShouldReport: true}}
 	fakeCli := &fakeDoghouseServerCli{}
 	fakeCli.FakeCheck = func(ctx context.Context, req *doghouse.CheckRequest) (*doghouse.CheckResponse, error) {
 		return &doghouse.CheckResponse{CheckedResults: wantResults}, nil
diff --git a/difffilter/filter_test.go b/difffilter/filter_test.go
index 64ba17cd..5a7840dd 100644
--- a/difffilter/filter_test.go
+++ b/difffilter/filter_test.go
@@ -135,7 +135,7 @@ func TestDiffFilter_root(t *testing.T) {
 			mode:         ModeAdded,
 			want:         false,
 			wantFileDiff: true,
-			wantLineDiff: false,
+			wantLineDiff: true,
 		},
 		{
 			path:         "sample.new.txt",
diff --git a/filter_test.go b/filter_test.go
index 346aa565..1a7cfded 100644
--- a/filter_test.go
+++ b/filter_test.go
@@ -78,6 +78,18 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 				},
 			},
 		},
+		{
+			Diagnostic: &rdf.Diagnostic{
+				Message: "outside range (start)",
+				Location: &rdf.Location{
+					Path: "sample.new.txt",
+					Range: &rdf.Range{
+						Start: &rdf.Position{Line: 1},
+						End:   &rdf.Position{Line: 2},
+					},
+				},
+			},
+		},
 	}
 	want := []*FilteredCheck{
 		{
@@ -89,10 +101,11 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: false,
-			InDiffFile:   true,
-			OldPath:      "sample.old.txt",
-			OldLine:      1,
+			ShouldReport:  false,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -103,11 +116,11 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: true,
-			InDiffFile:   true,
-			LnumDiff:     3,
-			OldPath:      "sample.old.txt",
-			OldLine:      0,
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       0,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -118,10 +131,11 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: false,
-			InDiffFile:   true,
-			OldPath:      "nonewline.old.txt",
-			OldLine:      1,
+			ShouldReport:  false,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "nonewline.old.txt",
+			OldLine:       1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -132,11 +146,30 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: true,
-			InDiffFile:   true,
-			LnumDiff:     5,
-			OldPath:      "nonewline.old.txt",
-			OldLine:      0,
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "nonewline.old.txt",
+			OldLine:       0,
+		},
+		{
+			CheckResult: &CheckResult{
+				Diagnostic: &rdf.Diagnostic{
+					Message: "outside range (start)",
+					Location: &rdf.Location{
+						Path: "sample.new.txt",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 1},
+							End:   &rdf.Position{Line: 2},
+						},
+					},
+				},
+			},
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       1,
 		},
 	}
 	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContent))
@@ -184,11 +217,11 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: true,
-			InDiffFile:   true,
-			LnumDiff:     1,
-			OldPath:      "sample.old.txt",
-			OldLine:      1,
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       1,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -199,11 +232,11 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: true,
-			InDiffFile:   true,
-			LnumDiff:     3,
-			OldPath:      "sample.old.txt",
-			OldLine:      0,
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       0,
 		},
 		{
 			CheckResult: &CheckResult{
@@ -214,11 +247,11 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 					},
 				},
 			},
-			ShouldReport: true,
-			InDiffFile:   true,
-			LnumDiff:     4,
-			OldPath:      "sample.old.txt",
-			OldLine:      0,
+			ShouldReport:  true,
+			InDiffFile:    true,
+			InDiffContext: true,
+			OldPath:       "sample.old.txt",
+			OldLine:       0,
 		},
 	}
 	filediffs, _ := diff.ParseMultiFile(strings.NewReader(diffContent))
diff --git a/service/github/github_test.go b/service/github/github_test.go
index 1f868b4c..55b75bad 100644
--- a/service/github/github_test.go
+++ b/service/github/github_test.go
@@ -79,7 +79,7 @@ func TestGitHubPullRequest_Post(t *testing.T) {
 					},
 				},
 			},
-			LnumDiff: 17,
+			InDiffContext: true,
 		},
 		Body: "[reviewdog] test",
 	}
@@ -212,9 +212,9 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 		default:
 			cs := []*github.PullRequestComment{
 				{
-					Path:     github.String("reviewdog.go"),
-					Position: github.Int(1),
-					Body:     github.String(commentutil.BodyPrefix + "\nalready commented"),
+					Path: github.String("reviewdog.go"),
+					Line: github.Int(2),
+					Body: github.String(commentutil.BodyPrefix + "\nalready commented"),
 				},
 			}
 			w.Header().Add("Link", `<https://api.github.com/repos/o/r/pulls/14/comments?page=2>; rel="next"`)
@@ -224,9 +224,21 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 		case "2":
 			cs := []*github.PullRequestComment{
 				{
-					Path:     github.String("reviewdog.go"),
-					Position: github.Int(14),
-					Body:     github.String(commentutil.BodyPrefix + "\nalready commented 2"),
+					Path: github.String("reviewdog.go"),
+					Line: github.Int(15),
+					Body: github.String(commentutil.BodyPrefix + "\nalready commented 2"),
+				},
+				{
+					Path:      github.String("reviewdog.go"),
+					StartLine: github.Int(15),
+					Line:      github.Int(16),
+					Body:      github.String(commentutil.BodyPrefix + "\nmultiline existing comment"),
+				},
+				{
+					Path:      github.String("reviewdog.go"),
+					StartLine: github.Int(15),
+					Line:      github.Int(16),
+					Body:      github.String(commentutil.BodyPrefix + "\nmultiline existing comment (line-break)"),
 				},
 			}
 			if err := json.NewEncoder(w).Encode(cs); err != nil {
@@ -254,9 +266,18 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 		}
 		want := []*github.DraftReviewComment{
 			{
-				Path:     github.String("reviewdog.go"),
-				Position: github.Int(14),
-				Body:     github.String(commentutil.BodyPrefix + "\nnew comment"),
+				Path: github.String("reviewdog.go"),
+				Side: github.String("RIGHT"),
+				Line: github.Int(15),
+				Body: github.String(commentutil.BodyPrefix + "\nnew 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 + "\nmultiline new comment"),
 			},
 		}
 		if diff := pretty.Compare(want, req.Comments); diff != "" {
@@ -279,10 +300,15 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 					Diagnostic: &rdf.Diagnostic{
 						Location: &rdf.Location{
 							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 2,
+								},
+							},
 						},
 					},
 				},
-				LnumDiff: 1,
+				InDiffContext: true,
 			},
 			Body: "already commented",
 		},
@@ -292,10 +318,15 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 					Diagnostic: &rdf.Diagnostic{
 						Location: &rdf.Location{
 							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+							},
 						},
 					},
 				},
-				LnumDiff: 14,
+				InDiffContext: true,
 			},
 			Body: "already commented 2",
 		},
@@ -305,10 +336,15 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 					Diagnostic: &rdf.Diagnostic{
 						Location: &rdf.Location{
 							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+							},
 						},
 					},
 				},
-				LnumDiff: 14,
+				InDiffContext: true,
 			},
 			Body: "new comment",
 		},
@@ -318,10 +354,75 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 					Diagnostic: &rdf.Diagnostic{
 						Location: &rdf.Location{
 							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+								End: &rdf.Position{
+									Line: 16,
+								},
+							},
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "multiline existing comment",
+		},
+		{
+			Result: &reviewdog.FilteredCheck{
+				CheckResult: &reviewdog.CheckResult{
+					Diagnostic: &rdf.Diagnostic{
+						Location: &rdf.Location{
+							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line:   15,
+									Column: 1,
+								},
+								End: &rdf.Position{
+									Line:   17,
+									Column: 1,
+								},
+							},
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "multiline existing comment (line-break)",
+		},
+		{
+			Result: &reviewdog.FilteredCheck{
+				CheckResult: &reviewdog.CheckResult{
+					Diagnostic: &rdf.Diagnostic{
+						Location: &rdf.Location{
+							Path: "reviewdog.go",
+							Range: &rdf.Range{
+								Start: &rdf.Position{
+									Line: 15,
+								},
+								End: &rdf.Position{
+									Line: 16,
+								},
+							},
+						},
+					},
+				},
+				InDiffContext: true,
+			},
+			Body: "multiline new comment",
+		},
+		{
+			Result: &reviewdog.FilteredCheck{
+				CheckResult: &reviewdog.CheckResult{
+					Diagnostic: &rdf.Diagnostic{
+						Location: &rdf.Location{
+							Path: "reviewdog.go",
+							// No Line
 						},
 					},
 				},
-				// No LnumDiff.
 			},
 			Body: "should not be reported via GitHub Review API",
 		},
@@ -391,7 +492,7 @@ func TestGitHubPullRequest_Post_toomany(t *testing.T) {
 						},
 					},
 				},
-				LnumDiff: i,
+				InDiffContext: true,
 			},
 			Body:     "comment",
 			ToolName: "tool",