GitHub pull request review #702

Antondomashnev · 2017-01-22T14:29:16Z

Hi, there this PR addresses an issue I opened a while ago #684.
The status is kinda work in progress, but there is already functionality that can be potentially used.

The changes in this PR:

new module GitHubSource to combine the only GitHub related features;
new class Review under the GitHubSource module, which represents the pull request review object;
extension to octokit which I think will be used until the API is not final and then I will create a PR to octokit itself with extension for pr reviews;

Here is an example of how it looks like in Dangerfile:

github.review.start
github.review.fail("I can fail your PR in a moment")
github.review.warn("I'm warning yo")
github.review.message("100 message")
github.review.submit

You can check how it looks in a PR here

The open points I want to discuss:

The overall design approach
Do we need the summary message in review? My intention to create it was to write something if the PR is well done and danger approves it without any comment.
Do you think it could be used without inline comments as a first step or you prefer the feature with inline comments?

P.S. the changelog is not modified yet and I haven't cleaned the commit history yet.

orta

This looks 👍 to me. I'm fine without doing line/file comments at first, as long as we document it as such

orta · 2017-01-22T18:41:52Z

lib/danger/request_sources/github/github_review_resolver.rb

+        end
+
+        # @orta @JuanitoFatas to be discussed whether we should submit review with
+        # the same status as already submitted by danger before


on an PR update? I think that's 👍

Done in 5730d68

orta · 2017-01-22T18:42:09Z

lib/danger/request_sources/github/octokit_pr_review.rb

+      post "#{Repository.path repo}/pulls/#{pull_request_number}/reviews/#{review_id}/events", options.merge(review)
+    end
+  end
+end


orta · 2017-01-22T18:52:26Z

lib/danger/danger_core/plugins/dangerfile_github_plugin.rb

+    # @!group PR Review
+    # Current PR Review object.
+    # @return [GitHubReview]
+    #


This will need considerable documentation before we ship ( as this is what an end user would see on the website ) - I'm happy to do this once you're ready though 👍

Sure, do you think we should provide an example here or what are your thoughts? 😄

Sure thing, something like this

# @!group PR Review # # In Beta. Provides access to creating a GitHub Review instead of a typical GitHub comment. # # To use you announce the start of your review, and the end via the `start` and `submit` functions, # for example: # # ``` # github.review.start # github.review.fail("Please add a CHANGELOG entry") if has_no_changelog # github.review.warn("Highway to the Danger Zone") if pr_includes_word_danger # github.review.message("You might want to read #{url}") if may_require_docs # github.review.submit # ``` # # @return [GitHubReview]

People don't see the inline documentation for the review object, so it needs to be attached to this DSL attribute to end up on the reference - http://danger.systems/reference.html

@orta looks cool 👍
The small feedback:

@return [GitHubReview] -> @return [GitHubSource::Review] or we can just leave Review.

Should we mention github.review.markdown(...) ?

@return [GitHubReview] -> @return [GitHubSource::Review] or we can just leave Review.

IMO @return Review is fine, maybe even @return ReviewDSL as these docs are for end-users who don't care about our internal semantics

Should we mention github.review.markdown(...) ?

Yep :D - please make it a real-ish example like the ones above

orta · 2017-01-22T18:52:43Z

lib/danger/request_sources/github/github_review.rb

+      class Review
+        include Danger::Helpers::CommentsHelper
+
+        # @see https://developer.github.com/v3/pulls/reviews/ for all possbile events


Fixed in 5730d68

orta · 2017-01-22T22:47:02Z

The overall design approach

This feels 👍 to me

Do we need the summary message in review? My intention to create it was to write something if the PR is well done and danger approves it without any comment.

I think approval without comment is perfectly fine IMO, if this does't feel right, we can change that at a later date. Note you should check if that's possible before making the assumption it is possible.

Do you think it could be used without inline comments as a first step or you prefer the feature with inline comments?

Fine by me

Antondomashnev · 2017-01-23T09:37:10Z

I think approval without comment is perfectly fine IMO, if this doesn't feel right, we can change that at a later date. Note you should check if that's possible before making the assumption it is possible.

Currently, it looks like this for example:

But yeah, maybe less "spammy" messages in PR - better 😄

orta · 2017-01-23T15:09:59Z

Yeah, if a review can be posted with 0 comments, I think it's worth adding instead of the redundant one there

orta · 2017-01-23T15:12:19Z

Yeah, like this one ^

JuanitoFatas · 2017-01-24T16:47:01Z

Rubocop is not happy 👮 Other than offenses, 👍 Great work!

Antondomashnev · 2017-01-24T20:27:51Z

@JuanitoFatas yep, I've noticed it, but I'm a bit confused now why it's green on my local machine with the same command.
Do you have any idea why it could happen?

I've also created a test #704 with up to date master and it fails with rubocop as well, really weird.

orta · 2017-01-24T21:19:26Z

Do you have any idea why it could happen?

I've always wondered if Rubocop actively ignores the fact that you're using bundler with it

Antondomashnev · 2017-01-25T20:43:29Z

Aha, that could be the case rubocop/rubocop#3968
I wanna try to downgrade rubocop to 0.46 and see what happens.

DangerCI · 2017-01-25T20:49:32Z

	1 Warning
⚠️	.gemspec modified

Generated by 🚫 danger

Antondomashnev · 2017-01-25T20:51:41Z

Yes the latest rubocop release of 0.47.1 was the issue.

orta · 2017-01-25T22:03:14Z

Alright - this looks good to me, lets get this shipped.

orta · 2017-01-25T22:21:53Z

Awesome - shipped as 4.1.0

Antondomashnev · 2017-01-25T22:34:47Z

@orta thanks, could you explain why the continuous-integration/appveyor/pr is red? Never worked with it 😄

orta · 2017-01-25T22:56:28Z

It runs our CI on windows, it's been having issues on all my OSS - might be capacity issues

orta reviewed Jan 22, 2017

View reviewed changes

Antondomashnev added 19 commits January 22, 2017 23:23

Working on github review integration;

2ac42ef

Playing around with github_review integration;

38465ee

Working on Octokit PullRequestReviews extension;

528494b

Create basic methods for PullRequestReivews for Octokit;

c3b362d

Fixes in sytax and logic in github & github_review classes;

a3c089d

Working on github review transitions resolver;

3cf1015

Add tests for review method for github request_source;

9c3165a

Add tests for review method for github request_source;

fbb4027

Add tests for github_review_resolver;

cb6e0c4

Update review resolver with new logic;

f427bee

Do not submit pull request review with the same body;

8f6daa9

Cleanup GitHubReview;

8ec776a

Create ReviewResolver tests;

6e612c6

Working on review spec;

b0149e0

Fix GitHubReview specs;

898e6a9

Finalize spec for github review;

b5bd8f6

Small cleanup in review object;

b07ef94

Fix testd and offenses;

d4a20ab

PR feedback; Test fixes;

5730d68

Antondomashnev force-pushed the antondomashnev/github_review branch from 799bd80 to 5730d68 Compare January 22, 2017 22:23

orta approved these changes Jan 23, 2017

View reviewed changes

Antondomashnev added 2 commits January 24, 2017 08:20

Do not show pull request review summary message;

c60b63f

Update changelog and documentation;

68474f0

Downgrade rubocop to 0.46;

4500e98

Antondomashnev changed the title ~~GitHub pull request review [WIP]~~ GitHub pull request review Jan 25, 2017

orta merged commit 98cd768 into danger:master Jan 25, 2017

Antondomashnev deleted the antondomashnev/github_review branch January 25, 2017 22:20

macklinu mentioned this pull request Aug 21, 2017

Support inline comments danger/danger-js#77

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub pull request review #702

GitHub pull request review #702

Antondomashnev commented Jan 22, 2017 •

edited

orta left a comment

orta Jan 22, 2017

Antondomashnev Jan 22, 2017

orta Jan 22, 2017

orta Jan 22, 2017

Antondomashnev Jan 22, 2017

orta Jan 22, 2017

orta Jan 22, 2017

Antondomashnev Jan 23, 2017

orta Jan 23, 2017

orta Jan 22, 2017

Antondomashnev Jan 22, 2017

orta commented Jan 22, 2017

Antondomashnev commented Jan 23, 2017 •

edited

orta commented Jan 23, 2017 •

edited

orta commented Jan 23, 2017

JuanitoFatas commented Jan 24, 2017

Antondomashnev commented Jan 24, 2017 •

edited

orta commented Jan 24, 2017

Antondomashnev commented Jan 25, 2017 •

edited

DangerCI commented Jan 25, 2017 •

edited

Antondomashnev commented Jan 25, 2017 •

edited

orta commented Jan 25, 2017

orta commented Jan 25, 2017 •

edited

Antondomashnev commented Jan 25, 2017

orta commented Jan 25, 2017

GitHub pull request review #702

GitHub pull request review #702

Conversation

Antondomashnev commented Jan 22, 2017 • edited

orta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta commented Jan 22, 2017

Antondomashnev commented Jan 23, 2017 • edited

orta commented Jan 23, 2017 • edited

orta commented Jan 23, 2017

JuanitoFatas commented Jan 24, 2017

Antondomashnev commented Jan 24, 2017 • edited

orta commented Jan 24, 2017

Antondomashnev commented Jan 25, 2017 • edited

DangerCI commented Jan 25, 2017 • edited

Antondomashnev commented Jan 25, 2017 • edited

orta commented Jan 25, 2017

orta commented Jan 25, 2017 • edited

Antondomashnev commented Jan 25, 2017

orta commented Jan 25, 2017

Antondomashnev commented Jan 22, 2017 •

edited

Antondomashnev commented Jan 23, 2017 •

edited

orta commented Jan 23, 2017 •

edited

Antondomashnev commented Jan 24, 2017 •

edited

Antondomashnev commented Jan 25, 2017 •

edited

DangerCI commented Jan 25, 2017 •

edited

Antondomashnev commented Jan 25, 2017 •

edited

orta commented Jan 25, 2017 •

edited