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

[Fix #233] Skip reviewing PR without comments #235

Merged
merged 1 commit into from
May 13, 2017

Conversation

ivanovaleksey
Copy link
Contributor

Hi @mmozuras,

I tried to fix #233.

Not sure about spec. It seems like spec/pronto/github_spec.rb uses RSpec contexts to describe behaviour. And I believe it is supposed to do it.

Please take a look.


context 'with comments' do
before do
github.should_receive(:pull_id).once.and_return(pull_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it OK to stub this method and not to deal with ENV['PRONTO_PULL_REQUEST_ID']?

Copy link
Member

Choose a reason for hiding this comment

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

@ivanovaleksey yup, I think that's fine.

@@ -39,6 +39,8 @@ def create_pull_comment(comment)
end

def create_pull_request_review(comments)
return if comments.empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another place for adding this could be GithubPullRequestReviewFormatter#submit_comments.
Not sure which one should be used.

Copy link
Member

@mmozuras mmozuras May 13, 2017

Choose a reason for hiding this comment

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

@ivanovaleksey I think that it makes sense here. If we'd call the client from someplace else, it should not submit either, not just in this case. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was what I thought about.

@mmozuras mmozuras merged commit 955b3b2 into prontolabs:master May 13, 2017
@SuperTux88
Copy link

This is now fixed since 1,5 month, can we please have a release for this?

@denschub
Copy link

denschub commented Jul 6, 2017

@mmozuras ping?

@mmozuras
Copy link
Member

mmozuras commented Jul 15, 2017

@SuperTux88 @denschub sorry about that. Had a rough month in my personal life, which made contributing to OSS more than difficult. This again shows why completing #227 should be a priority.

@mmozuras
Copy link
Member

Released as part of v0.9.4.

@denschub
Copy link

denschub commented Jul 15, 2017

In that case: sorry for being annoying. Hope you are better and thank you very much for your work. It is much appreciated. 😻

@mmozuras
Copy link
Member

mmozuras commented Jul 15, 2017

@denschub no worries, I understand that hearing nothing from me could've been frustrating 😄. And thank you for the kind words 🙇.

apiology pushed a commit to apiology/pronto that referenced this pull request Dec 27, 2019
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.

GitHub PR Review formatter console warning
4 participants