Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

prevent comment line out of diff #117

Merged
merged 3 commits into from
Dec 14, 2020

Conversation

Nov1kov
Copy link
Contributor

@Nov1kov Nov1kov commented Dec 11, 2020

Motivation

I have a problem with Gitlab implementation.

NewLine 150, OldLine150
org.gitlab4j.api.GitLabApiException: 400 (Bad request) "Note {:line_code=>["can't be blank", "must be a valid line code"]}" not given
	at org.gitlab4j.api.AbstractApi.validate(AbstractApi.java:633)
	at org.gitlab4j.api.AbstractApi.post(AbstractApi.java:286)
	at org.gitlab4j.api.DiscussionsApi.createMergeRequestDiscussion(DiscussionsApi.java:332)
	at se.bjurr.violations.comments.gitlab.lib.GitLabCommentsProvider.createSingleFileComment(GitLabCommentsProvider.java:225)
	at se.bjurr.violations.comments.lib.CommentsCreator.createSingleFileComments(CommentsCreator.java:163)
	at se.bjurr.violations.comments.lib.CommentsCreator.createComments(CommentsCreator.java:85)
	at se.bjurr.violations.comments.lib.CommentsCreator.createComments(CommentsCreator.java:40)
	at se.bjurr.violations.comments.gitlab.lib.ViolationCommentsToGitLabApi.toPullRequest(ViolationCommentsToGitLabApi.java:177)
	at se.bjurr.violations.comments.gitlab.plugin.gradle.ViolationCommentsToGitLabTask.gitChangelogPluginTasks(ViolationCommentsToGitLabTask.java:211)

So, after research, I made this pull request.

Detailes

  1. Method isLineInDiff was work incorrectly. I changed check newlines by another map, but it shows for us map newLineToLineInDiffTable works incorrectly. See below the next steps for another PR.
  2. It isn't a problem for me, but old line numbers in map newLineToOldLineTable didn't parse correctly.

Next steps for another PR.

As far as I understand the method findLineInDiff is for GitHub implementation.

GitHub doc: https://docs.github.com/en/free-pro-team@latest/rest/reference/pulls#create-a-review-comment-for-a-pull-request
Note: The position value equals the number of lines down from the first "@@" hunk header in the file you want to add a comment. The line just below the "@@" line is position 1, the next line is position 2, and so on. The position in the diff continues to increase through lines of whitespace and additional hunks until the beginning of a new file.

So,

  1. The map newLineToLineInDiffTable didn't work for deleted lines.
  2. The map newLineToLineInDiffTable didn't work for multi changes in one file (many @@ sections in one file)
  3. If it map newline -> to count diffs. It should skip deleted lines.
    I'll show for my PR example:
    image

@tomasbjerre
Copy link
Owner

Thanks! I realize I have not tested this part properly. I added some approval tests in master, like this:
688266f

It would help understand this change. To see how those results differ.

So I'd like you to rebase, or merge, with my master and update the test cases.

@Nov1kov
Copy link
Contributor Author

Nov1kov commented Dec 13, 2020

After merge I had a broken test. testPatchApprovalChangedDiff2
I edited mock diff to best understand what happened. I hope it's not a problem for the repository. So, I edited one excepted result.

@tomasbjerre tomasbjerre merged commit c5fd3fb into tomasbjerre:master Dec 14, 2020
tomasbjerre added a commit to tomasbjerre/violation-comments-lib that referenced this pull request Dec 14, 2020
tomasbjerre added a commit to tomasbjerre/violation-comments-to-gitlab-lib that referenced this pull request Dec 14, 2020
tomasbjerre added a commit to jenkinsci/violation-comments-to-gitlab-plugin that referenced this pull request Dec 14, 2020
tomasbjerre added a commit to tomasbjerre/violation-comments-to-gitlab-gradle-plugin that referenced this pull request Dec 14, 2020
tomasbjerre added a commit to tomasbjerre/violation-comments-to-gitlab-maven-plugin that referenced this pull request Dec 14, 2020
tomasbjerre added a commit to tomasbjerre/violation-comments-to-gitlab-command-line that referenced this pull request Dec 14, 2020
@tomasbjerre
Copy link
Owner

This is released now! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants