Merge pull request #698 from reviewdog/github-non-line-based-suggestion

github-pr-review: Support range suggestions

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

_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"}]}

_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 ↑

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

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,
 		},

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
+}