News for hackers. Stuff that matters.
Re:Google takes the pain out of code reviews, with 97% dev s... by cgearhart on Monday December 04, 2023 @11:57AM (#38519718)
This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes.

As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is worse than finding it with tests is worse than finding it at your desk is worse than finding it in the design. Engineering teams should choose an operating point in their processes that balances quality and velocity to match the business needs of their project. Most often I’ve seen slow PRs caused by stuffing too much of the responsibility for software quality into that single step.

In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it. We minimize trivial nits by enforcing a style checker. We also discuss in retro if the PRs are too big to review effectively or if PRs are turning up comments on solution architecture, because at that point it’s too late—the author likely should’ve separately discussed the architecture before it got to a PR.

Re:Google takes the pain out of code reviews, with 97% dev s... by zdragnar on Monday December 04, 2023 @12:27PM (#38520149)
> In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it

This is different from what the OP is talking about.

I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previous PR.

Oh, and you had to address every one of them before getting approval.

Re:Google takes the pain out of code reviews, with 97% dev s... by Reubend on Monday December 04, 2023 @01:49PM (#38521326)
> I've worked at places where staff engineers seem to have been rated on number of comments left on PRs

> Oh, and you had to address every one of them before getting approval.

This just sounds horrible.

Re:Google takes the pain out of code reviews, with 97% dev s... by d0mine on Monday December 04, 2023 @02:01PM (#38521507)
> rated on the number of comments

It sounds idiotic.

It is unrelated to the question whether short-term (one PR) only reviews are preferable over reviews that recognize the reality that it is not the last PR ever and therefore more long-term view may be useful.

Re:Google takes the pain out of code reviews, with 97% dev s... by wubrr on Monday December 04, 2023 @12:01PM (#38519787)
Generally agree, but at some companies/teams the ratio of 'nitpicky subjective feedback' to 'you have a bug here feedback' is like 95:5 while at others it's 5:95. The latter function much better.
Re:Google takes the pain out of code reviews, with 97% dev s... by poolopolopolo on Monday December 04, 2023 @12:03PM (#38519807)
MRs are bad place to align things at scale.
Re:Google takes the pain out of code reviews, with 97% dev s... by creshal on Monday December 04, 2023 @01:06PM (#38520688)
They should be a last resort for that sort of alignment, but it's still important to pull that last resort if it's necessary, so you don't end up like Cloudflare and let your own standards slip so badly that you end up with a 100% preventable multi-day outage.
Re:Google takes the pain out of code reviews, with 97% dev s... by hot_gril on Monday December 04, 2023 @01:19PM (#38520870)
Long-term project health rests on bigger things like API design, database choices, or service layout.
Re:Google takes the pain out of code reviews, with 97% dev s... by uoaei on Monday December 04, 2023 @01:21PM (#38520906)
Hopefully these are in-scope when it comes to accepting or rejecting PRs and are not just considered incidental to whatever the stated purpose of the PR is. What you listed appears to be more fundamental system design concerns so hopefully they are candidates for further review.
Re:Google takes the pain out of code reviews, with 97% dev s... by hot_gril on Monday December 04, 2023 @01:41PM (#38521201)
It's usually out of scope by the time you're writing a CL, which is fine because there's design review. The issue is about priorities. Nitpicky code review can be very expensive, and I'm not convinced it brings noticeable benefit.

I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.

Re:Google takes the pain out of code reviews, with 97% dev s... by throwawaaarrgh on Monday December 04, 2023 @03:14PM (#38522450)
> it starts showing cracks on a long enough timeline in sufficiently large or complex projects

Show me a project which this doesn't happen to on a long timeline and I'll show you a project with no users and perfect programmers.

Entropy is unavoidable. It will eventually degrade despite your best efforts and in the meantime you aren't benefiting from a working feature. We should try to delay software entropy as much as practical, but not at the expense of shipping.

Basically there is a balance to strike. I don't have a formula for it, but I do know that merging often ends up being more valuable than merging later just to ensure clean code. Working features are more valuable than a mess. A mess sucks, so we should try to avoid it, but the world runs on messes, not clean code.

Re:Google takes the pain out of code reviews, with 97% dev s... by danielovichdk on Monday December 04, 2023 @04:25PM (#38523327)
Read the Redis code base please.