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

71–80 of 302 posts

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

#71

Post by tallowen »

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.

I don't think the burden of the changing API surface was place on the callers of that API. With core library developers updating their callsites, it meant that people calling into early versions of React, never had to deal with upgrading the react version in their package. As a caller of these libraries, it was much nicer to have the React team take care of upgrades where possible rather than having to deal with it myself.

Furthermore, there were tons of changes that could happen across the board at facebook that didn't happen at google due to owners files. Lots of people were empowered to make broad changes to the entire codebase which in aggregate made the codebase better. Coming from Facebook, all the systems around owners and readability represented a huge amount of time to be able to make changes that should have been easy. It also left the codebase with many warts that would have been relatively quick to fix locally but a pain to land.

Facebook went in the other direction - there were better and better tools for making broad updates

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

#72

Post by michaelt »

shadowgovt wrote: post #7
jdeaton wrote: post #5

Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.

What's the current miss-rate on them? In principle, it seems brilliant, but I can see that value dropping off a cliff if more than half of them are worthless.

> What's the current miss-rate on them?

Depends how good your code is :)

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

#73

Post by tallowen »

jeffbee wrote: post #27
marcinzm wrote: post #22

Earlier quoted context omitted.

The same applies to Facebook which is their comparison and not some random 10 person startup.

Except Facebook's codebase is an order of magnitude smaller.

What makes you say that Facebook's codebase is an order of magnitude smaller?

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

#74

Post by nox100 »

_ea1k wrote: post #52
sanderjd wrote: post #36

Earlier quoted context omitted.

I appreciate suggestions that are not "directly related to a necessary change to the code", and I believe my coworkers appreciate mine. It is very useful to say "did you consider doing it xyz way / using xyz library?" while also mashing the "approve" button. If they say "yep and I decided not to because of xyz" or "yep I like that idea but I'm not gonna change it at this point" then no harm done. But pretty often in…

> [...] 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 allow minor edits but it's strict enough that I've rarely seen it pass an edit. I assume there must have been some incident that triggered this.

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

#75

Post by marcosdumay »

BytesAndGears wrote: post #44
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…

I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow”…

I've received plenty of negative feedback on the internet for this (and up to now, discarded all), but I really don't think acceptance review is the correct time for guidance.

Yes, juniors need guidance. That means you must read their code, and talk to them about it. They also need real-world feedback. They do really need you not managing all of the interaction they have with the real world. Now, what you can not manage is up for a complex risk analysis, but they really need you to get out of the way at some point.

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

#76

Post by bheadmaster »

tikhonj wrote: post #30
bheadmaster wrote: post #23

Earlier quoted context omitted.

"Readable" and "misleading" are subjective, and depend on the person's "preconceptions". I disagree that the reviewer comes "without as many preconceptions" - they just come with different preconceptions, the ones they're used to. Programming language is a language like any other, and each person has their own style of writing it, and they'd prefer the rest of the world to use their own style because it's "more reada…

Are you saying there is no such thing as clearer or less clear writing in natural languages or programming languages? It's 100% subjective and depends entirely on the reader?

Of course, it's not 100% subjective - rarely anything in the world is 100% anything.

But I think that most people consider their personal preferences to be better and more readable just because they're used to them, so I tend to take the opposite attitude as the starting point. There were more than a few situations where I've had a coworker tell me "just read the code, it's very readable", only to spend the next two weeks just trying to figure out how it works. Sure, once you figure out how it works and it "clicks", it's no longer (that) unreadable, but the fact that I have to spend so much time reading the codebase in the first place made me convinced that personal familiarity is a great part of what "readable" means.

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

#77

Post by ryandrake »

arp242 wrote: post #60
shadowgovt wrote: post #17

Earlier quoted context omitted.

Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.

But "another team is relying on not to move" is an objective point, right? Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer". I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone poi…

I'd wonder what the rest of that API surface looked like. If the rest of the functions were longDescriptiveCamelCase() and then you tried to slip in mkdir(), then I'd say yes, "stylistic consistency" was gained during that unnecessarily difficult back-and-forth.

If the other functions were chdir(), remove(), rmdir() and so on, and somehow the reviewers picked your code change as the time to change it all, then yea, what a waste.

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

#78

Post by Kelkonosemmel »

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…

I communicate on purpose that code review is never the 99% mark

It's the time to get feedback and improve.

The problem is: if people are stuck and don't learn the code review phase will always stay as a long thing.

The whole team is responsible for security, maintainability, etc.

I don't care about some code monkey able to write some code or fixing something. I care about a team, a product.

Your attitude would not work in my team, which is fine

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

#79

Post by lokar »

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…

IME this is always driven by lazy compliance people pushing the most straight forward way to satisfy some audit. Plenty of places subject to strict audit rules manage to make small post review edits work.
Post reply on HN