Use Gitlab suggestions in merge request comments #842
Conversation
|
Not sure why CircleCI lint is failing: |
f2e7f22 to
bf783f5
Compare
|
GitLab actually have code suggestions feature, so I'd like to support code-suggestion instead of just showing diff. Can you work on it? I'm still not sure but you can ignore the circle lint ci failure. I guess it's due to permission issue (PR from forked repo). |
|
@haya14busa indeed, thanks for pointing out. Should be possible to implement, I'll try to make it work |
|
Looks beautiful now! |
|
@mgrachev or @haya14busa could you please take a look? |
|
circleci complains about the following: This is the step that fails: reviewdog/.circleci/config.yml Line 18 in 207b245 |
|
@jetersen I think that's because of incorrect permissions, since PR is made from the forked repo. Maybe it's something to adjust in the repository settings? Locally it's complaining for number of issues for the code I haven't changed: ❯ golint ./... | reviewdog -f=golint --filter-mode=nofilter
comment_iowriter.go:17:1: exported function NewRawCommentWriter should have comment or be unexported
comment_iowriter.go:21:1: exported method RawCommentWriter.Post should have comment or be unexported
comment_iowriter.go:40:1: exported function NewUnifiedCommentWriter should have comment or be unexported
comment_iowriter.go:44:1: exported method UnifiedCommentWriter.Post should have comment or be unexported
diff.go:11:6: exported type DiffString should have comment or be unexported
diff.go:16:1: exported function NewDiffString should have comment or be unexported
diff.go:20:1: exported method DiffString.Diff should have comment or be unexported
diff.go:24:1: exported method DiffString.Strip should have comment or be unexported
diff.go:30:6: exported type DiffCmd should have comment or be unexported
diff.go:38:1: exported function NewDiffCmd should have comment or be unexported
diff.go:60:1: exported method DiffCmd.Strip should have comment or be unexported
diff.go:67:1: exported method EmptyDiff.Diff should have comment or be unexported
diff.go:71:1: exported method EmptyDiff.Strip should have comment or be unexported
resultmap.go:17:6: exported type Result should have comment or be unexported
resultmap.go:77:6: exported type FilteredResult should have comment or be unexported
cienv/github_actions.go:9:1: comment on exported type GitHubEvent should be of the form "GitHubEvent ..." (with optional leading article)
cienv/github_actions.go:27:6: exported type GitHubRepo should have comment or be unexported
cienv/github_actions.go:33:6: exported type GitHubPullRequest should have comment or be unexported
doghouse/appengine/github.go:26:6: exported type GitHubHandler should have comment or be unexported
doghouse/appengine/github.go:40:1: exported function NewGitHubHandler should have comment or be unexported
doghouse/appengine/github.go:103:1: exported method GitHubHandler.HandleAuthCallback should have comment or be unexported
doghouse/appengine/github.go:140:1: exported method GitHubHandler.HandleLogout should have comment or be unexported
doghouse/appengine/github.go:145:1: exported method GitHubHandler.LogInHandler should have comment or be unexported
doghouse/appengine/github.go:222:1: exported method GitHubHandler.HandleGitHubTop should have comment or be unexported
doghouse/appengine/github.go:376:1: exported function NewAuthClient should have comment or be unexported
doghouse/appengine/github_webhook.go:89:1: comment on exported type InstallationEvent should be of the form "InstallationEvent ..." (with optional leading article)
doghouse/appengine/github_webhook.go:101:1: comment on exported type CheckSuiteEvent should be of the form "CheckSuiteEvent ..." (with optional leading article)
doghouse/client/github_client.go:18:1: exported method GitHubClient.Check should have comment or be unexported
doghouse/server/doghouse.go:33:6: exported type Checker should have comment or be unexported
doghouse/server/doghouse.go:38:1: exported function NewChecker should have comment or be unexported
doghouse/server/doghouse.go:42:1: exported method Checker.Check should have comment or be unexported
doghouse/server/github.go:12:6: exported type NewGitHubClientOption should have comment or be unexported
doghouse/server/github.go:25:1: exported function NewGitHubClient should have comment or be unexported
doghouse/server/token.go:12:1: exported function GenerateRepositoryToken should have comment or be unexported
doghouse/server/token.go:22:1: exported function GetOrGenerateRepoToken should have comment or be unexported
doghouse/server/token.go:35:1: exported function RegenerateRepoToken should have comment or be unexported
doghouse/server/ciutil/ciutil.go:85:1: comment on exported function UpdateTravisCIIPAddrs should be of the form "UpdateTravisCIIPAddrs ..."
doghouse/server/ciutil/ciutil.go:106:1: exported function IPFromReq should have comment or be unexported
doghouse/server/storage/installation.go:64:1: exported method GitHubInstallationDatastore.Get should have comment or be unexported
doghouse/server/storage/token.go:51:1: exported method GitHubRepoTokenDatastore.Get should have comment or be unexported
filter/filter.go:38:6: func name will be used as filter.FilterCheck by other packages, and that stutters; consider calling this Check
parser/checkstyle.go:21:1: exported method CheckStyleParser.Parse should have comment or be unexported
parser/errorformat.go:35:1: exported method ErrorformatParser.Parse should have comment or be unexported
project/run_test.go:63:28: error strings should not be capitalized or end with punctuation or a newline
service/commentutil/commentutil.go:12:1: comment on exported type PostedComments should be of the form "PostedComments ..." (with optional leading article)
service/github/githubutils/comment_writer.go:14:7: exported const MaxLoggingAnnotationsPerStep should have comment or be unexported
service/github/githubutils/comment_writer.go:31:1: exported method GitHubActionLogWriter.Post should have comment or be unexported
service/github/githubutils/comment_writer.go:82:1: exported function WarnTooManyAnnotationOnce should have comment or be unexported |
|
I noticed that last merged MR had the same issue: #825, circleci job: https://app.circleci.com/pipelines/github/reviewdog/reviewdog/2884/workflows/3eaff01a-d69b-4981-9d4f-0f1f508ff24c/jobs/6844 |
| @@ -77,6 +77,35 @@ func MarkdownComment(c *reviewdog.Comment) string { | |||
| return sb.String() | |||
| } | |||
|
|
|||
| // MarkdownSuggestions creates diff in markdown for suggested changes | |||
| func MarkdownSuggestions(c *reviewdog.Comment) string { | |||
There was a problem hiding this comment.
This markdown syntax (suggestion block) is not common but it's specific to GitLab. Can you move this func to gitlab file?
| if suggestionsRendered > 0 { | ||
| sb.WriteString("\n\n") | ||
| } | ||
| sb.WriteString("```suggestion:-0+") |
There was a problem hiding this comment.
Suggestion position (lines) is not always the same as the position of the diagnostic.
Can you test all possible cases including multiple lines suggestions (including E2E testing)? You can refer to the test of GitHub.
It might be difficult to support all cases, but we shouldn't post wrong suggestions as much as possible.
| @@ -77,6 +77,35 @@ func MarkdownComment(c *reviewdog.Comment) string { | |||
| return sb.String() | |||
| } | |||
|
|
|||
| // MarkdownSuggestions creates diff in markdown for suggested changes | |||
There was a problem hiding this comment.
Can you add a link to the GitLab document?
Maybe https://docs.gitlab.com/ee/user/discussions/#multi-line-suggestions
| sb.WriteString("```suggestion:-0+") | ||
| sb.WriteString(fmt.Sprintf("%d", s.Range.End.Line-s.Range.Start.Line)) | ||
| sb.WriteString("\n") | ||
| sb.WriteString(strings.Trim(s.GetText(), "\n")) |
There was a problem hiding this comment.
You should not trim the last line-break.
Suggestion can contain the line-break and/or blank lines at the last.
| if suggestionsRendered > 0 { | ||
| sb.WriteString("\n\n") | ||
| } | ||
| sb.WriteString("```suggestion:-0+") |
There was a problem hiding this comment.
Probably it's safer to use 4 backticks if it also works when the suggestion doesn't contain code-fence block?
https://docs.gitlab.com/ee/user/discussions/#code-block-nested-in-suggestions
Can you check?
|
@haya14busa thanks a lot for a careful review! I agree with all the points you addressed and will aim to implement the changes. |
|
Hi @caseycs, did you have any chance to look further into this topic? Would be very interested to test this out with GitLab. |
|
I created a new pull request #1012 to continue work on that issue |
|
This is merged by #1012 |
I noticed that the issue found by diff parser is triggering a comment in Gitlab on the correct line but without any explanation, So I decided to append comment with suggested changes in markdown diff format.