Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A study of Google's code review tooling (Critique)

engineercodex.substack.com

Post reply on HN
Print view View original

81–90 of 302 posts

Re: A study of Google's code review tooling (Critique)

#81

Post by dkersten »

nox100 wrote: post #74
_ea1k wrote: post #52

Earlier quoted context omitted.

> [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.

It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allo…

I read it as it’s up to the person who opened the PR whether to apply the nits, or whether to ignore them and use the approval to merge. This is how our codebase works: you’re free to ignore minor suggestions, but if you do edit the code, reviews are dismissed and need to be provided again.

Basically, an unaddressed nit doesn’t block merging. But any code changes will require fresh reviews.

If the change is very minor, it’s very quick for someone who has already approved to check the last commit diff and reapprove.

Re: A study of Google's code review tooling (Critique)

#82

Post by lokar »

throwawaaarrgh wrote: post #14

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

IME the biggest blocking feedback to Jr engineers about how to better structure things (the most expensive feedback) is around making it easy and clean to test the code. Fairly often a modest (but straightforward) refactor is needed to get good tests.

Re: A study of Google's code review tooling (Critique)

#83

Post by tgsovlerkhgsel »

jrockway wrote: post #53
throwawaaarrgh wrote: post #14

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message a…

Everyone thinks their task is the most important one. If the "lock OS until review done" thing were a thing, there's a good chance that "lock OS until random bullshit someone else wants you to do" would keep you from sending out the code for review in the first place.

And even though it's "just a quick thing", if every time you're trying to get something done some "quick thing" gets in the way, it also gets incredibly frustrating. [1][2] Finding a good balance for this is hard.

What helps in my experience is building a reputation for small, easy to review changes. I know that I've left reviews from one coworker sitting for a long time because I either already took a look and knew that it was, or hadn't taken a look and dreaded it to be, a huge block of code that would require significant comments.

[1] https://www.reddit.com/r/ProgrammerHumor/comments/2rmir6/why... [2] https://www.reddit.com/r/ProgrammerHumor/comments/pafo1v/und...

Re: A study of Google's code review tooling (Critique)

#84

Post by wubrr »

sanderjd wrote: post #48
onlyrealcuzzo wrote: post #32

Earlier quoted context omitted.

Code review at most companies is an ego thing, for reviewers to feel self-important. It's a way for senior people to pieces of crap to everyone underneath them because that's what they put up with for most of their career - and now they feel like it's their turn to be a piece of crap back and perpetuate the cycle. So many industries run on this exact same cycle. At Google, the company does a good job convincing you t…

I never know whether I've just been fortunate in my career or what, but I've just never experienced anything approaching the kind of unkindness that I frequently see people say is common. I dunno, maybe it's just me, but I'm skeptical. I've always worked with professional colleagues who are trying to figure out how to do their best, and have never once seen people behaving in this childish way.

It really depends on the company/team. Certain companies (Amazon), use PR feedback stats as promotion metrics. They don't even consider the feedback quality, literally the stat 'average number of comments per PR' was one of the metrics used at my last team. The only 2 people promoted on this team during my tenure there got promoted based mostly on their loudness in PRs and team meetings and constant pushing for various (mostly counterproductive) 'process improvements' - these people didn't actually ship a single feature/project on the team. Very toxic place to work to say the least.

Re: A study of Google's code review tooling (Critique)

#85

Post by tuetuopay »

jeffbee wrote: post #9

Satisfaction with Critique among Xooglers is undoubtedly driven by dissatisfaction with GitHub PR reviews. GitHub reviews are astonishingly bad. The tool is utterly useless for actual reviews. After the first round of comments it becomes total chaos. Nobody can tell what's been said, done, changed, or resolved. It is impossible to believe that the people who write and maintain the GitHub PR tools have themselves ever…

I get the same feeling when switching between Gitlab and Github. The latter is a joke of usability and discoverability.

Tiny details like thread filtering, thread replies not appearing as separate comments, "show changed lines" when a new commit modifies the subject of a comment, etc. Those add up to make gitlab much more usable.

Re: A study of Google's code review tooling (Critique)

#86

Post by zdragnar »

cgearhart wrote: post #62
throwawaaarrgh wrote: post #14

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

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 wo…

> 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: A study of Google's code review tooling (Critique)

#87

Post by CalChris »

ASinclair wrote: post #29
tallowen wrote: post #13

I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your P…

> I don’t think libraries like React could get developed at Google given how hard it would be to evolve the API surface. That's more of a feature and not a bug to me. Users of a library would appreciate it if the API surface doesn't change dramatically over time.

Dramatically, no. But then a couple of releases of deprecated and then hasta la vista is fine with me if it means progress.

Re: A study of Google's code review tooling (Critique)

#88

Post by wubrr »

ryandrake wrote: post #64
Eji1700 wrote: post #47

Earlier quoted context omitted.

> If you are holding up the approval because you want someone to rewrite the code the way you would have written it, get out of the way. While I'm being pedantic here and get your point, I do think it's important that code is following proper styles. I get that there's a bunch of nitpicky nonsense that goes into code review, but making sure it follows the same style so in 6 months when someone else has to dig through…

Style wars are the kind of thing that should just be automated away with a source control hook that reformats all submissions.

This. Any subjective style/pattern agreed to by the team should be checked and/or fixed automatically. If you don't have lint rules set up to enforce these things - go write them instead of commenting about subjective style preferences in PRs.

Re: A study of Google's code review tooling (Critique)

#89

Post by dawnerd »

throwawaaarrgh wrote: post #14

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

Yeah no, part of code review is ensuring code quality is maintained. If you start letting badly written code fly because it passes tests, that’ll bite you. If something isn’t blocking I’ll make a note that it should be fixed if theres budget knowing sometimes you gotta let things go. But you can’t just ignore code quality and call it nitpicking.

Re: A study of Google's code review tooling (Critique)

#90

Post by hyggetrold »

I think it's funny that devs will go through all kinds of untold suffering when it comes to code reviews...stacked pull requests, waiting days (or more!) for a review, all kinds of nitpicky BS that is a lot of time and effort to rework by the time you have the code all written.

But suggest to people that they pair program and that it's a real-time review that obviates the need for formal async review and folks want to murder you.

Post reply on HN