Skip to content
This repository was archived by the owner on Jul 19, 2025. It is now read-only.

Conversation

jyurek
Copy link
Contributor

@jyurek jyurek commented Mar 16, 2015

No description provided.

end

def add_comment
if config.add_comment && !comment_present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the add_comment check removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are actually relying on these checks to be here (and in #update_status) in our other PR 😄 #55

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay - we resolved our differences and are no longer relying on the config check that @jyurek removed here - so #55 should be compatible with this PR now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marshally Because the check actually happens here: https://github.com/codeclimate/codeclimate-services/blob/jy-do-not-allow-nil-branches/lib/cc/services/github_pull_requests.rb#L53 and this check only served to muddy the waters about what was happening.

Add a middleware that should be placed at the bottom of the Invocation
stack. This middleware ensures that any time the invocation stack is
called, there is a return value. If the stack comes back nil, then a
message is put in its place implying an error. While this might not
technically be the case that the service errored, a lack of return value
should be considered an error and looked in to.
@jyurek
Copy link
Contributor Author

jyurek commented Mar 17, 2015

Talked with @marshally on the phone. He gives a :shipit: by proxy.

jyurek pushed a commit that referenced this pull request Mar 17, 2015
Give an OK of false if the PR comment exists
@jyurek jyurek merged commit 6bb2a12 into master Mar 17, 2015
@jyurek jyurek deleted the jy-do-not-allow-nil-branches branch March 17, 2015 17:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants