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

Comment only violations that relate to a diff #27

Closed
tuukkamustonen opened this issue Oct 31, 2016 · 6 comments
Closed

Comment only violations that relate to a diff #27

tuukkamustonen opened this issue Oct 31, 2016 · 6 comments

Comments

@tuukkamustonen
Copy link

It seems like the plugin now writes all violation comments that relate to a file in the diff.

The comments that don't have actual diff are left in the Overview-tab in the main stream (instead of showing up under Diff-tab).

It would be better to write just those comments, that relate to code that was actually changed?

@tomasbjerre
Copy link
Contributor

Yes that would be better. I did not manage to figure out how to determine if a reported line is changed in the pull request, or not.

The GitHub API was easier. There I made it optional to comment all changed files, or comment only changed parts of files: https://github.com/tomasbjerre/violation-comments-to-github-lib/blob/master/src/main/java/se/bjurr/violations/comments/github/lib/GitHubCommentsProvider.java#L112

@tomasbjerre
Copy link
Contributor

You may say that you encourage the boy scout rule =) So developers should clean up any file they touch.

@tuukkamustonen
Copy link
Author

You may say that you encourage the boy scout rule =)

Ha 😄! I guess it's feasible and circumvents the problem.

I'm not really familiar with Atlassian APIs, but doesn't /rest/api/1.0/projects/{projectKey}/repos/{repositorySlug}/pull-requests/{pullRequestId}/diff at Bitbucket server REST API (anchors don't appear to work on that page so no direct link) seem to drive the need?

Documentation is a bit misleading, as you would first think that srcPatch is required parameter, but it actually isn't.

@tomasbjerre
Copy link
Contributor

I think it works now in 1.34 but I did not have time to test it alot.

Lines can be reported as ADDED, REMOVED or CONTEXT. I decided to include ADDED and CONTEXT. Because a change on line 10 may trigger a violation on line 9...

And to check if a line is in CONTEXT, the rest call is something like http://localhost:7990/rest/api/1.0/projects/TOM/repos/violations-test/pull-requests/1/diff.

tomasbjerre added a commit that referenced this issue Nov 1, 2016
@tomasbjerre
Copy link
Contributor

Actually I changed it again. 1.35 only includes ADDED. And there is a field, context, for specifying how many lines surrounding an added line that should be included.

@tuukkamustonen
Copy link
Author

Sounds feasible, I'll give it a try. Thanks again!

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

No branches or pull requests

2 participants