Allow individual violations to be acknowledged, and ignored for a given PR

[prpetten]

Idea

We'd like a 1 time override on a specific violation found by Hound that we could implement by replying to Hound with an ignore message for that specific violation on that specific commit only.

FAQs

Q1 Why not just turn off this check in the lint config?
A1 We all agree its a valuable check and should be followed 99.9% of the time.

Q2 Why not just exclude this file in the lint config?
A2 We all agree its a valuable check, and we want it checking all other lines in this file for this violation.

Q3 Why not use inline comments to tell the linter to ignore that line?
A3 Because sometimes we don't want to go through that trouble for legacy code that we're cleaning up in the first place.

Q4 Why not ignore the failing check, and merge the Pull Request anyway?
A4 Because we don't really want to get into the habit of ignoring failing checks, even if we know what they're caused by, and we've elected to ignore them. This sets a bad precedence.

Q5 Why not set fail_on_violations = false in .hound.yml?
A5 That leads to the cognitive dissonance of a green check mark with 10 violations found., and its misleading when reviewing. (Note: this is our current practice)

Q6 Why not just fix the mother-bleeping violation?
A6 Because sometimes that requires a major refactoring of legacy code, and we'd like to be pragmatic about how we spend our time.

Q7 Isn't this just a slippery slope? Why have hound at all, if all you need to do is tell it to shut up?
A7

• Because we are, in fact, lazy, and we'd rather not make all the changes Hound is so kindly recommending. So, we use the exception to the rule, as an excuse not to use Hound at all.
• Being the lazy creatures we are, we're unlikely to use this feature repeatedly, as it will typically be less work to fix the core issue and push up our new commit than it will be to have a conversation about why this time should be an exception.
• We still want to retain our veto power over our Hound overlords.

Proposed Workflow

1. Create/Update a Pull Request, and Hound dutifully comments on your egregious style error like Joan Rivers (RIP) on the Fashion Police

So here is a sample issue, obviously one we would fix rather than override, but it is just for an example.

2. Reply to message with #hushpuppy, or some other please ignore message

3. Hound updates PR to account for ignored messages

Note: If there is a single outstanding violation, the note for hound will be update to say 1 violation found. 1 violation ignored, and the status will update to a green check. If there are still more outstanding violations that do not have a reply to hound to ignore, the status would remain a big fat red x.

[jcmorrow]

@prpetten Great write-up! I think this is an interesting idea and I'm curious if other users would also use it. One question I do have is if this

Q6 Why not just fix the mother-bleeping violation?
A6 Because sometimes that requires a major refactoring of legacy code, and we'd like to be pragmatic about how we spend our time.

is actually the use case. If hound is commenting on things that require an actual refactor, then I feel like it might be checking too much (hound should only comment on style, not function, so it shouldn't require a refactor to implement the feedback). However, I have seen scenarios where people are OK with violations in certain places (E.G. they're ok with migration lines being longer than 80 characters, same for spec description lines) that are too specific as to be configurable via rubocop, but which also shouldn't count as a failing violation for a PR. Just my two cents.

[prpetten]

@jcmorrow Thanks for the feedback. I wish I had thought of the line going over length, as we actually have our max_line_length = 150, but our policy internally is that they shouldn't be more than 120 characters, just to get around those occasions where a couple of extra characters is much clearer than to create a new line.

In terms of the refactoring being required, the cops I'm thinking of are:

• Metrics/AbcSize
• Metrics/BlockNesting
• Metrics/ClassLength
• Metrics/CyclomaticComplexity
• Metrics/MethodLength
• Metrics/ParameterLists
• Metrics/PerceivedComplexity

[gylaz]

While I think this is a good idea, the amount of work it would take to properly implement this would be substantial. I think the right move forward is to leave this open and see how much interest something like this would generate.

In the meanwhile, we tend to ignore the comments that Hound makes that we're okay letting through (happens here and there) and we don't fail the build on style violations.

[prpetten]

@gylaz When you say, "we don't fail the build on style violations", do you mean you set fail_on_violations = false in .hound.yml, or is there some more granular setting where "style" Hound violations do not fail the build, but non-"style" Hound violations do fail the build?

[gylaz]

I meant, we don't set fail_on_violations in .hound.yml, which defaults to false.

[prpetten]

@gylaz Thanks, I didn't realize fail_on_violations wasn't required in .hound.yml, or that it defaults to false.

[vaskaloidis]

I mistakingly had Hound added as a CI service (apparently, before I really understood the extend of it's involvement...) I did not think it would comment on every single line for this merge I just attempted... For the same exact issue about a thousand times... Extremely overwhelming / frustrating / annoying and now I think I am stuck with all of them..

I guess I should have done more research before adding Hound to my GH project. I really really love all the other Thoughtbot products / projects, so I assumed Hound would help keep contributors code clean - but the fact that it commented on every line-change about a simple indentation issue and a quote vs double quote issue, of my commit is extremely annoying, and I wish there was a way to undo it.

On a separate note - what is the point of having it make about a thousand comments complaining about incorrect indentation and a thousand comments complaining about single vs double quotes... It could have conveniently placed a single comment, or a single message somewhere, rather than spamming every single line-addition with this comment.. Extremely frustrating... What is the reasoning behind allowing Hound to make the same comment (about a lexical error) more than 10-20 times?? Especially when you cannot reverse them...

[gylaz]

For the same exact issue about a thousand times

Hound has a limit of making 16 comments per review of PR. If you have a review with thousands of comments, can you refer us to that PR (if public) and we'll take a look at how it was possible.

and now I think I am stuck with all of them..
and I wish there was a way to undo it.

You can always just delete the comments in GitHub.

On a separate note - what is the point of having it make about a thousand comments complaining about incorrect indentation and a thousand comments complaining about single vs double quotes... It could have conveniently placed a single comment, or a single message somewhere, rather than spamming every single line-addition with this comment.. Extremely frustrating... What is the reasoning behind allowing Hound to make the same comment (about a lexical error) more than 10-20 times?? Especially when you cannot reverse them...

This sounds like vetting, and not productive contribution... The point of Hound is to comment on the exact line where a style guide violation was found, for ease of integration into the code review process. Typically, this is a few violation comments per PR. As, I've said, thousands should not be possible.

[vaskaloidis]

I apologize if it came off as unproductive, my only reason for mentioning this was to bring-up that this issue was affecting me, in hopes that it gets resolved makes the product better.

Like I said, I love Thoughtbot and I felt like this experience was not up-to-par with the quality I have come to expect from Thoughtbot products.

As for not vetting the product, you are correct, in retrospect I did not vet it correctly - I had assumed that Hound would work like other CI products I have used in the past, and give me it's results in the form of a report. I see now that it specifically says it will comment on each violation.

I just wanted to mention how I thought Hound could be better: I think Hound should report its findings differently, rather than commenting. I feel like other people might agree with me, but I would assume that many people just removed the integration from GitHub without ever mentioning their grievances. I thought I would contribute, and mention how I thought Hound could work better, based on my initial experience.

Edit: I assume that somebody will point me towards a different product that may report style violations differently (like a report). That is fine, but I still wanted to leave a comment about what I thought / expected Hound would be, versus what it actually was once I used it.

[salbertson]

I might have missed some context, but this is basically supported by GitHub now.