search

jenkinsci / violation-comments-to-stash-plugin

Comments Bitbucket Server (or Stash) pull requests with static code analyzer findings.
https://plugins.jenkins.io/violation-comments-to-stash
MIT License
43 stars 16 forks source link

Comment only violations that relate to a diff #27

Closed tuukkamustonen closed 8 years ago

tuukkamustonen commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

tuukkamustonen commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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