Comments on PR with success message if configured #201
Conversation
| end | ||
| end | ||
| context 'when the pull request is not open' do | ||
| it "does not comment on the pull request" do |
There was a problem hiding this comment.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
| @@ -1,5 +1,6 @@ | |||
| class PullRequest | |||
| CONFIG_FILE = '.hound.yml' | |||
| SUCCESS_MESSAGE = 'No style issues found! :+1:' | |||
There was a problem hiding this comment.
I think saying "No style issues found." is probably enough.
There was a problem hiding this comment.
Actually, we've been calling them violations, so "No style violations found."
|
I'm unsure on this. Many developers tend to push multiple times to an open PR. This solution would cause multiple success comments posted to the PR. Another alternative that we've thrown around is to use the GitHub status bar, but then we'll be contending for it with TravisCI, CodeClimate, and other services that use it. |
| @@ -49,6 +49,11 @@ | |||
| RedundantBegin | |||
| Enabled: false | |||
|
|
|||
| # Enable SuccessMessage if you want Hound to comment when there | |||
| # are no style violations. | |||
| SuccessMessage | |||
There was a problem hiding this comment.
Thoughts on calling it something like NoViolationsFound? (since it's not obvious what success is in the case of Hound).
There was a problem hiding this comment.
Yes, I think including violations is more clear. Thoughts on NoViolationsComment?
There was a problem hiding this comment.
It's probably a good idea to keep more general, say in case we stop commenting and start displaying something in the GitHub status. So... maybe NoViolationNotification? Though, I'm not a fan of the the negation prefix, I'd love to find a positive way to say this. I don't know if NotifyOfNoViolations is any better.
I'm almost tempted to go back to SuccessNotification or someting.
There was a problem hiding this comment.
Hm, this is tough. The negative prefix could indeed be interpreted differently. If I had to vote I'd go for SuccessNotification. Especially if this was integrated with Github status and the fact that Hound performs builds, 'success' seems to make sense to me.
|
Will this solution cause multiple comments if Hound runs multiple times for a single pull request? |
|
If enabled, yes, this would comment for every push to an open pull request. |
|
Is there a way to tell which success comment is for which commit? Do the comments collapse when new commits are pushed to the pull request? |
I left a 'demo' comment just now. You can see that it includes the commit hash in the comment:
Unfortunately, no. Do you think deleting all previous 'success' comments, if present, would be a decent solution? Then you would only see one 'success' comment for the latest successful push. |
|
@hstove I don't think deleting comments is a good idea. Hound should be running and reporting on violations within seconds unless there is something wrong, like we've hit our rate limit or jobs are backed up. I appreciate the input and the code but I'm not sure how useful a "success" message is to other users. I'll open an issue to continue the conversation. |
|
No problem! Thanks for the great code review! |
|
Of course, anytime. |
Here's the problem: I am an expert programmer who makes 0 mistakes
. When I create a pull request without any style violations, I get stuck in 'limbo' waiting for comments from Hound. There is no way for me to know whether this is because I don't have Hound setup correctly, or because Hound is down, or because there were no violations.
My proposed solution is the ability for the user to add a special option to
.hound.yml, like so:When present, Hound will comment with 'No style issues found! 👍'.
I personally think that this behavior should be the default instead of opt-in, but I will leave that to the maintainers to decide.
Finally, this code base is a joy to work on! ❤️