Repository: reviewdog/reviewdog
Pull request #698 (2020-07-25 17:31:54)

github-pr-review: Support range 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/CHANGELOG.md b/CHANGELOG.md
index be91f3ff..ac97d844 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,7 +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
+ - [#675](https://github.com/reviewdog/reviewdog/pull/675) [#698](https://github.com/reviewdog/reviewdog/pull/698) github-pr-review: Support suggested changes
 - ...
 
 ### :bug: Fixes
diff --git a/filter/filter.go b/filter/filter.go
index 848faa31..409ecfa1 100644
--- a/filter/filter.go
+++ b/filter/filter.go
@@ -17,8 +17,14 @@ type FilteredDiagnostic struct {
 	// If it's a multiline result, both start and end must be in the same diff
 	// hunk.
 	InDiffContext bool
-	OldPath       string
-	OldLine       int
+
+	// Source lines text of the diagnostic message's line-range.
+	// It contains a whole line even if the diagnostic range have column fields.
+	// Optional. Currently available only when it's in diff context.
+	SourceLines []string
+
+	OldPath string
+	OldLine int
 }
 
 // FilterCheck filters check results by diff. It doesn't drop check which
@@ -37,11 +43,15 @@ func FilterCheck(results []*rdf.Diagnostic, diff []*diff.FileDiff, strip int,
 			endLine = startLine
 		}
 		check.InDiffContext = true
+		sourceLines := []string{}
 		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 diffline != nil {
+				sourceLines = append(sourceLines, diffline.Content)
+			}
 			if difffile != nil {
 				check.InDiffFile = true
 				if l == startLine {
@@ -50,7 +60,9 @@ func FilterCheck(results []*rdf.Diagnostic, diff []*diff.FileDiff, strip int,
 				}
 			}
 		}
-		// loc.Path = NormalizePath(loc.GetPath(), cwd, "")
+		if check.InDiffContext {
+			check.SourceLines = sourceLines
+		}
 		checks = append(checks, check)
 	}
 	return checks
diff --git a/service/github/github.go b/service/github/github.go
index fc9005ad..50d3cd96 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -23,6 +23,11 @@ var _ reviewdog.DiffService = &GitHubPullRequest{}
 
 const maxCommentsPerRequest = 30
 
+const (
+	invalidSuggestionPre  = "<details><summary>reviewdog suggestion error</summary>"
+	invalidSuggestionPost = "</details>"
+)
+
 // GitHubPullRequest is a comment and diff service for GitHub PullRequest.
 //
 // API:
@@ -261,7 +266,7 @@ func buildSuggestions(c *reviewdog.Comment) string {
 	for _, s := range c.Result.Diagnostic.GetSuggestions() {
 		txt, err := buildSingleSuggestion(c, s)
 		if err != nil {
-			sb.WriteString(fmt.Sprintf("Invalid suggestion: %v", err))
+			sb.WriteString(invalidSuggestionPre + err.Error() + invalidSuggestionPost + "\n")
 			continue
 		}
 		sb.WriteString(txt)
@@ -276,12 +281,11 @@ func buildSingleSuggestion(c *reviewdog.Comment, s *rdf.Suggestion) (string, err
 	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",
+		return "", fmt.Errorf("the Diagnostic's lines and Suggestion lines must be the same. L%d-L%d v.s. L%d-L%d",
 			drange.GetStart().GetLine(), drange.GetEnd().GetLine(), start.GetLine(), end.GetLine())
 	}
 	if start.GetColumn() > 0 || end.GetColumn() > 0 {
-		// TODO(haya14busa): Support non-line based suggestion.
-		return "", errors.New("non line based suggestions (contains column) are not supported yet")
+		return buildNonLineBasedSuggestion(c, s)
 	}
 	var sb strings.Builder
 	sb.WriteString("```suggestion\n")
@@ -292,3 +296,30 @@ func buildSingleSuggestion(c *reviewdog.Comment, s *rdf.Suggestion) (string, err
 	sb.WriteString("```")
 	return sb.String(), nil
 }
+
+func buildNonLineBasedSuggestion(c *reviewdog.Comment, s *rdf.Suggestion) (string, error) {
+	sourceLines := c.Result.SourceLines
+	slen := len(sourceLines)
+	if slen == 0 {
+		return "", errors.New("source lines are not available")
+	}
+	start := s.GetRange().GetStart()
+	end := s.GetRange().GetEnd()
+	if slen != int(end.GetLine()-start.GetLine()+1) {
+		return "", errors.New("invalid source lines: not all source lines for this suggestion are available")
+	}
+	var sb strings.Builder
+	sb.WriteString("```suggestion\n")
+	sb.WriteString(sourceLines[0][:max(start.GetColumn()-1, 0)])
+	sb.WriteString(s.GetText())
+	sb.WriteString(sourceLines[slen-1][max(end.GetColumn()-1, 0):])
+	sb.WriteString("\n```")
+	return sb.String(), nil
+}
+
+func max(x, y int32) int32 {
+	if x < y {
+		return y
+	}
+	return x
+}


Test patch:
diff --git a/_testdata/suggestions.go b/_testdata/suggestions.go
deleted file mode 100644
index 17316f2c..00000000
--- a/_testdata/suggestions.go
+++ /dev/null
@@ -1,4 +0,0 @@
-package testdata
-
-func suggestions() {
-}
diff --git a/_testdata/suggestions.json b/_testdata/suggestions.json
index cbd23a47..663926ed 100644
--- a/_testdata/suggestions.json
+++ b/_testdata/suggestions.json
@@ -1,2 +1,10 @@
-{"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"}]}
+{"message":"suggestion test","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":3}}}, "suggestions": [{"range":{"start":{"line":3}},"text": "func Suggestions() {"}]}
+{"message":"multiline suggestion test","location":{"path":"_testdata/suggestions.txt","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"}]}
+{"message":"range suggestion test (replace)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}}, "suggestions": [{"range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}, "text": "14"}]}
+{"message":"suggestion test (multiple suggestions)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}}, "suggestions": [{"range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}, "text": "14"},{"range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}, "text": "1"},{"range":{"start":{"line":6,"column":5},"end":{"line":6,"column":7}}, "text": "2"}]}
+{"message":"range suggestion test (insert)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":7,"column":5},"end":{"line":7,"column":5}}}, "suggestions": [{"range":{"start":{"line":7,"column":5},"end":{"line":7,"column":5}}, "text": "14"}]}
+{"message":"range suggestion test (remove)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":8,"column":5},"end":{"line":8,"column":7}}}, "suggestions": [{"range":{"start":{"line":8,"column":5},"end":{"line":8,"column":7}}, "text": ""}]}
+{"message":"range suggestion test (remove line-break)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":10,"column":5},"end":{"line":11,"column":1}}}, "suggestions": [{"range":{"start":{"line":10,"column":5},"end":{"line":11,"column":1}}, "text": ""}]}
+{"message":"suggestion test (remove line)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":13}}}, "suggestions": [{"range":{"start":{"line":13}}, "text": ""}]}
+{"message":"suggestion test (insert line)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":16,"column":1},"end":{"line":16,"column":1}}}, "suggestions": [{"range":{"start":{"line":16,"column":1},"end":{"line":16,"column":1}}, "text": "inserted line 1\ninserted line 2\n"}]}
+{"message":"suggestion test (invalid)","location":{"path":"_testdata/suggestions.txt","range":{"start":{"line":16}}}, "suggestions": [{"range":{"start":{"line":17}}, "text": "invalid"}]}
diff --git a/_testdata/suggestions.txt b/_testdata/suggestions.txt
new file mode 100644
index 00000000..a2e86080
--- /dev/null
+++ b/_testdata/suggestions.txt
@@ -0,0 +1,16 @@
+package testdata
+
+func suggestions() {
+}
+
+haya13busa (replace)
+hayabusa (insert)
+haya14busa (remove)
+
+--->
+<--- (remove line-break)
+
+remove line
+
+insert line ↓
+insert line ↑
diff --git a/filter/filter_test.go b/filter/filter_test.go
index 9113e8f4..1b931566 100644
--- a/filter/filter_test.go
+++ b/filter/filter_test.go
@@ -91,6 +91,7 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			ShouldReport:  false,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"unchanged, contextual line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       1,
 		},
@@ -104,6 +105,7 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"added line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       0,
 		},
@@ -117,6 +119,7 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			ShouldReport:  false,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{`" vim: nofixeol noendofline`},
 			OldPath:       "nonewline.old.txt",
 			OldLine:       1,
 		},
@@ -130,6 +133,7 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"b"},
 			OldPath:       "nonewline.old.txt",
 			OldLine:       0,
 		},
@@ -147,6 +151,7 @@ func TestFilterCheckByAddedLines(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"unchanged, contextual line", "added line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       1,
 		},
@@ -191,6 +196,7 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"unchanged, contextual line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       1,
 		},
@@ -204,6 +210,7 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"added line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       0,
 		},
@@ -217,6 +224,7 @@ func TestFilterCheckByDiffContext(t *testing.T) {
 			ShouldReport:  true,
 			InDiffFile:    true,
 			InDiffContext: true,
+			SourceLines:   []string{"added line"},
 			OldPath:       "sample.old.txt",
 			OldLine:       0,
 		},
diff --git a/service/github/github_test.go b/service/github/github_test.go
index bfa197d1..4ead8af2 100644
--- a/service/github/github_test.go
+++ b/service/github/github_test.go
@@ -313,8 +313,30 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 				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")),
+					invalidSuggestionPre + "the Diagnostic's lines and Suggestion lines must be the same. L15-L16 v.s. L16-L17" + invalidSuggestionPost,
+				}, "\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{
+					"non-line based suggestion comment (no source lines)",
+					invalidSuggestionPre + "source lines are not available" + invalidSuggestionPost,
+				}, "\n") + "\n"),
+			},
+			{
+				Path: github.String("reviewdog.go"),
+				Side: github.String("RIGHT"),
+				Line: github.Int(15),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"range suggestion (single line)",
+					"```suggestion",
+					"haya14busa",
+					"```",
+				}, "\n") + "\n"),
 			},
 			{
 				Path:      github.String("reviewdog.go"),
@@ -323,9 +345,52 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 				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")),
+					"range suggestion (multi-line)",
+					"```suggestion",
+					"haya14busa (multi-line)",
+					"```",
+				}, "\n") + "\n"),
+			},
+			{
+				Path:      github.String("reviewdog.go"),
+				Side:      github.String("RIGHT"),
+				StartSide: github.String("RIGHT"),
+				StartLine: github.Int(15),
+				Line:      github.Int(17),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"range suggestion (line-break, remove)",
+					"```suggestion",
+					"line 15 (content at line 15)",
+					"```",
+				}, "\n") + "\n"),
+			},
+			{
+				Path: github.String("reviewdog.go"),
+				Side: github.String("RIGHT"),
+				Line: github.Int(15),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"range suggestion (insert)",
+					"```suggestion",
+					"haya14busa",
+					"```",
+				}, "\n") + "\n"),
+			},
+			{
+				Path: github.String("reviewdog.go"),
+				Side: github.String("RIGHT"),
+				Line: github.Int(15),
+				Body: github.String(commentutil.BodyPrefix + "\n" + strings.Join([]string{
+					"multiple suggestions",
+					"```suggestion",
+					"haya1busa",
+					"```",
+					"```suggestion",
+					"haya4busa",
+					"```",
+					"```suggestion",
+					"haya14busa",
+					"```",
+				}, "\n") + "\n"),
 			},
 		}
 		if diff := pretty.Compare(want, req.Comments); diff != "" {
@@ -579,7 +644,155 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 							Text: "replacement",
 						},
 					},
-					Message: "non-line based suggestion comment",
+					Message: "non-line based suggestion comment (no source lines)",
+				},
+				InDiffContext: true,
+			},
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				SourceLines: []string{"haya15busa"},
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 15, Column: 5},
+							End:   &rdf.Position{Line: 15, Column: 7},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 15, Column: 7},
+							},
+							Text: "14",
+						},
+					},
+					Message: "range suggestion (single line)",
+				},
+				InDiffContext: true,
+			},
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				SourceLines: []string{
+					"haya???",
+					"???busa (multi-line)",
+				},
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 15, Column: 5},
+							End:   &rdf.Position{Line: 16, Column: 4},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 16, Column: 4},
+							},
+							Text: "14",
+						},
+					},
+					Message: "range suggestion (multi-line)",
+				},
+				InDiffContext: true,
+			},
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				SourceLines: []string{
+					"line 15 xxx",
+					"line 16",
+					"(content at line 15)",
+				},
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 15, Column: 9},
+							End:   &rdf.Position{Line: 17, Column: 1},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 9},
+								End:   &rdf.Position{Line: 17, Column: 1},
+							},
+							Text: "",
+						},
+					},
+					Message: "range suggestion (line-break, remove)",
+				},
+				InDiffContext: true,
+			},
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				SourceLines: []string{
+					"hayabusa",
+				},
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 15, Column: 5},
+							End:   &rdf.Position{Line: 15, Column: 5},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 15, Column: 5},
+							},
+							Text: "14",
+						},
+					},
+					Message: "range suggestion (insert)",
+				},
+				InDiffContext: true,
+			},
+		},
+		{
+			Result: &filter.FilteredDiagnostic{
+				SourceLines: []string{"haya??busa"},
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "reviewdog.go",
+						Range: &rdf.Range{
+							Start: &rdf.Position{Line: 15, Column: 5},
+							End:   &rdf.Position{Line: 15, Column: 7},
+						},
+					},
+					Suggestions: []*rdf.Suggestion{
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 15, Column: 7},
+							},
+							Text: "1",
+						},
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 15, Column: 7},
+							},
+							Text: "4",
+						},
+						{
+							Range: &rdf.Range{
+								Start: &rdf.Position{Line: 15, Column: 5},
+								End:   &rdf.Position{Line: 15, Column: 7},
+							},
+							Text: "14",
+						},
+					},
+					Message: "multiple suggestions",
 				},
 				InDiffContext: true,
 			},