Hacker News Reader

Comment by withinboredom | Hacker News Reader

I think it’s the “requiring” a review part that comes across as not trustworthy. A review isn’t required on my team, but we do it most of the time. There have been plenty of trivial changes I’ve made where getting a review would have been a waste of everyone’s time or just been “rubber stamped” anyway. I just my team and my team trusts me to make that judgement call.

View on HN

The problem is the subset of devs who don't know what they don't know.

Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it"

Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"

I thought you were going to say “now this string has to get retranslated and until then everyone will have a crappy experience” lol

Changing strings are about the most harmful thing to the bottom line where I work. :)

However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.

Haha, absolutely, it's a terrible example with terrible design that I pulled out of thin air, but it was just to illustrate the point of how even seemingly innocuous changes can have major consequences.

And the original author's point is that you can't possibly happen to foresee all of the consequences, and so the best thing to do is to just ship the change and deal with what breaks.

That's the whole point of continuous integration, and that's what the author is trying to push. That you actually move faster by letting people push and fix bugs versus push a change, wait some time for a code review (somewhere 1min...1day), go back and forth on some style things, maybe a the reviewer managed to win the game of "spot the bug," and then ship, and then fix any delinquent bugs.

Like, unless if that middle reviewer is doing something that really can't be done elsewhere, it just serves to slow things down.

> Actually you've removed a keyword that our error messaging framework uses

Hmm, so something is parsing a string looking for specific character sequences? I'd say the developer who changed that is doing the team a favor by surfacing a terrible practice.

Edit: NVM I see you acknowledged that in another comment.

The reason for requiring a review on a team with high trust is that you can trust your team to validate the assumption that a given change is “trivial”. My team has had several minor changes recently where mistakes were caught in the review process.

If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.

In many companies changes must be reviewed for compliance reasons (e.g., SOC2 will generally require some sort of review procedure).

Reply on news.ycombinator.com