diff --git a/.github/workflows/reviewdog.yml b/.github/workflows/reviewdog.yml index 6bd90fd633..d32764f114 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 059bd385eb..a11ce40db6 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/_testdata/custom.txt b/_testdata/custom.txt new file mode 100644 index 0000000000..9bda1ee0d7 --- /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 0000000000..70ab847706 --- /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 37acf66632..8dcae64994 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.go b/difffilter/filter.go index 2bc4ee2e98..e33cc4bf0f 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/difffilter/filter_test.go b/difffilter/filter_test.go index 64ba17cd5a..5a7840dd87 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.go b/filter.go index 94fe399464..09649c28b2 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/filter_test.go b/filter_test.go index 346aa5657e..1a7cfded3a 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.go b/service/github/github.go index 8e00e58a39..15fceca6ca 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 } diff --git a/service/github/github_test.go b/service/github/github_test.go index 1f868b4ccf..55b75bad37 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", `; 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",