Do not send status update if user disable it #57
Conversation
| receive_pull_request({}, { | ||
| github_slug: "pbrisbin/foo", | ||
| commit_sha: "abc123", | ||
| state: "pending", |
There was a problem hiding this comment.
this should be state: "error" to match the test description
There was a problem hiding this comment.
I fixed it but you commented the wrong one.
sikachu
force-pushed
the
ps-fix-update-status-disabled
branch
2 times, most recently
from
223dd3a to
fd55ad0
Compare
We forgot to check if user has turned on the `update_status` flag or not before we sends a pending status to GitHub. This commit includes refactoring, hopefully to cleanup the confusion on conditionals.
|
This is ready for another look. I updated the code to hopefully make it cleaner, and also add more tests on those default response ... Ready for re-review! |
| update_status_skipped | ||
| end | ||
| when "error" | ||
| update_status_error |
There was a problem hiding this comment.
Goodbye cruel conditionals!
|
LGTM |
1 similar comment
|
LGTM |
| end | ||
|
|
||
| def update_status_success | ||
| update_status("success", "Code Climate has analyzed this pull request.") | ||
| add_comment |
There was a problem hiding this comment.
Not in love with having status mixed in with commenting, but it's a larger issue with this class in general (that it does two things). Commenting should go away soon, so I'm ok with this.
However -- I think we swap the order of these lines for now. We're not capturing both results in the response back (just the last one), but the PR one is the more important one right now.
There was a problem hiding this comment.
Right. This class is doing too much.
I'm fine with the swap, but that means we're actually change the functionality of this (and its return value). Currently, we return the status of add comment only. Do you think we should swap it anyway?
There was a problem hiding this comment.
That's fair. Perhaps a separate PR then is appropriate for this change. I think it's in scope however, because we're now capturing the result of these calls -- and the most important result is the one that comes back from posting a status to a PR (not the comment).
There was a problem hiding this comment.
Yep. Let's do that swap in another PR. 🥚
Do not send PR status update if user disable it
…eady exists Trello: https://trello.com/c/npDQ4rN3/103-record-response-from-sending-commit-status-instead-of-adding-comment-when-both-commit-status-and-comment-are-on fixes: #57 (diff) GithubPullRequests were mistakenly returning a false success code in the common case where a comment is already present on the pull request being updated. We should return a success instead.
…eady exists Trello: https://trello.com/c/npDQ4rN3/103-record-response-from-sending-commit-status-instead-of-adding-comment-when-both-commit-status-and-comment-are-on fixes: #57 (diff) GithubPullRequests were mistakenly returning a false success code in the common case where a comment is already present on the pull request being updated. We should return a success instead.
We forgot to check if user has turned on the
update_statusflag or not before we sends a pending status to GitHub.This commit includes refactoring, hopefully to cleanup the confusion on conditionals.