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

Post individual comments when possible #82

Merged
merged 44 commits into from
Jun 1, 2016
Merged

Post individual comments when possible #82

merged 44 commits into from
Jun 1, 2016

Conversation

stkent
Copy link
Collaborator

@stkent stkent commented May 31, 2016

Fixes #34, Fixes #67

This is the PR I've been testing on:

https://github.com/stkent/gnag-testing/pull/1

It includes a subset of the files from the gnag example project. In particular, this allows testing of the following scenarios:

  • reporting violations where the location within the diff can be determined (violations in files from the example project included in the PR fit this case);
  • reporting violations where the location within the diff CANNOT be determined (violations in files from the example project NOT included in the PR fit this case).


@Override
public void onResponse(final Call<T> call, final Response<T> response) {
// This method intentionally left blank.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could log more info here, but as I said above, was not receiving any responses to inline comment POSTs anyway...

@stkent
Copy link
Collaborator Author

stkent commented Jun 1, 2016

@btkelly this is ready for review!

@stkent
Copy link
Collaborator Author

stkent commented Jun 1, 2016

screen shot 2016-06-01 at 9 03 43 am

Comments like this (same file, line number) could be consolidated. Can either do that in this PR or open a follow-up issue. Would improve performance also.

@stkent
Copy link
Collaborator Author

stkent commented Jun 1, 2016

Follow-up issue: avoid duplicating comments on subsequent CI builds (need to be careful about this + understand how it impacts comments being marked as outdated).


violationsWithValidLocationInfo.sort(Violation.COMMENT_POSTING_COMPARATOR);

violationsWithValidLocationInfo.stream()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java 8, so cool!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's like rxJava; just have to avoid overuse :P

@btkelly
Copy link
Owner

btkelly commented Jun 1, 2016

@stkent I love it! Only request would be to update the README screenshot to show the line commenting instead of the aggregated comments.

@stkent
Copy link
Collaborator Author

stkent commented Jun 1, 2016

Good call; on it!

@pr-bot
Copy link

pr-bot commented Jun 1, 2016

@btkelly updated

@btkelly btkelly merged commit 9b07f03 into master Jun 1, 2016
@btkelly btkelly deleted the individual-3 branch June 1, 2016 19:34
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.

3 participants