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

281–290 of 302 posts

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

#281

Post by cmrdporcupine »

crotchfire wrote: post #278
cmrdporcupine wrote: post #143

Earlier quoted context omitted.

I assume that Critque is Grerrit with a bunch of Google-specific changes. Not even close. I have another comment where I get into some details, but, no, three's no overlap beyond the fact that Gerrit pulled some UI and workflow things from Critique (and Mondrian before that, the tool that predated Critique)

I dunno. I use gerrit frequently and nothing in this article surprised me. Aside from "ML-powered woo woo" I've seen and used everything bragged about in this article. Gerrit is awesome. I will never, ever go back to github.

Whether you're surprised or not or like Gerrit is beside the point. I like it too. I was simply responding to your assertion that Critique is a fork or derived from Gerrit, which is not correct.

They are two entirely separate codebases, built on two entirely different revision controls systems -- one open source, the other not -- with Gerrit inspired by Critique, not the other way around. Yes there are similarities be tween it and Critique. Because Googlers worked on both.

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

#282

Post by jgoodknight »

ayberk wrote: post #172
dontreact wrote: post #3

I generally like the tool. When your code review tool is really nice one unexpected negative is that it can create a culture of nitpicking. Sometimes it’s not worth arguing over small details like variable naming but the tool makes it really easy for things to head in that direction. Sometimes variable naming isn’t a small detail. But sometimes it is and it’s a waste of everyone’s time to argue about it.

I don't know if it's the tool honestly. I've recently switched teams and I'm in the process of getting java readability. The whole experience so far has been much worse than getting my C++ readability. I even get "nit" comments on the code I haven't changed. I have had multiple comments where reviewer basically "preferred" one style over another. It's been still mostly helpful, but I've had my share of frustration. C…

That was my experience getting Java readability too! Only 1 time in 20 did it feel like they were making the code better

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

#283

Post by ruds »

withinboredom wrote: post #193
ravenstine wrote: post #169

Earlier quoted context omitted.

Kind of off topic but I'm curious what it is about a foreach that you think would do a better job than a syntactical for-loop. In my experience, for-loops are almost always the better choice, unless the implementation of "foreach" is done in a way that is functional and recursive. Foreach in the usual sense (like the method in JavaScript) provides too little control to the callee and is only really of benefit if one…

> what it is about a foreach that you think would do a better job than a syntactical for-loop It depends on the language. For example, an optimizing compiler can optimize the shit out of a for-each loop, but can only go so far in a for-loop. For example, in a for-loop, you can "skip around" the indices, and even go backward after going forward, while in a for-each loop, it will always go one-by-one, and the compiler…

Not only that, but a code reader can rely on this too. If you only use a raw for loop when you're skipping around, then the reviewer / future reader can slow down to understand what's happening here. If you use raw for loops for the for-each case, now the reader has to slow down on all your for loops. It's similar to the reasoning for "const everywhere": if you don't mark variables const, I need to figure out as a reader whether the variable is modified later, and that slows me down and increases the mental load of reading a function.

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

#285

Post by cutemonster »

acscott wrote: post #15
TheBlight wrote: post #8

Earlier quoted context omitted.

Excessive subjective feedback can be soul-crushing.

Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development

Sounds as if that happened suddenly ("got hit"), what changed (in the workplace) that made such bad things possible? (After not having happened for 20 years)

How long did it take until you felt better again? If I can ask

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

#286

Post by merb »

dgellow wrote: post #220
bluGill wrote: post #196

Earlier quoted context omitted.

Then please write one. Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that migh…

It’s called cspell https://cspell.org/

cspell works as a guide, but not as a rejection/auto correction. It’s okish but it does not solve intent. It‘s perfect for comments tough

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

#287

Post by troebr »

crotchfire wrote: post #278
cmrdporcupine wrote: post #143

Earlier quoted context omitted.

I assume that Critque is Grerrit with a bunch of Google-specific changes. Not even close. I have another comment where I get into some details, but, no, three's no overlap beyond the fact that Gerrit pulled some UI and workflow things from Critique (and Mondrian before that, the tool that predated Critique)

I dunno. I use gerrit frequently and nothing in this article surprised me. Aside from "ML-powered woo woo" I've seen and used everything bragged about in this article. Gerrit is awesome. I will never, ever go back to github.

I miss Gerrit from my last job. Stacked PRs on Github are horrible. Lots of problems stem from that: because stacked PRs are painful, people make large PRs, because the PRs are large, they take a long time to merge, because they take a long time to merge you need more rebases, needing more re-reviews etc.

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

#288

Post by froh »

mplanchard wrote: post #248
froh wrote: post #218

Earlier quoted context omitted.

I feel stupid. How do I do this?

When writing an inline comment, there’s a button to make a suggestion. It inserts a markdown code fence with the existing code on the line(s), which you can edit. When you submit the comment, it shows up with diff highlighting and can be applied by the author with one click. Essentially the same thing exists in both GitHub and GitLab

ah yes! duh. and thanks. I need better glasses...

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

#289

Post by Arainach »

withinboredom wrote: post #277
redditor98654 wrote: post #268

Earlier quoted context omitted.

I see. I want to try and understand this because I am also trying to get better at code reviews and not come across as a dogmatic person. I have spent almost 15 years in mostly AWS and I want to keep myself in check and make sure people don't take my suggestions as the vague "quality" as you so mention just because of my seniority. Here is the most recent PR I did for a relatively young person in my org. Part of the…

There are three parts to every code review: 1. Code style: such as formatting and when to use certain things (non-negotiable and you really should automate that). 2. Working code: does the PR have a description/ticket and does the code do what it promises to do? Can we refactor anything to make it better? 3. Conventions: does the PR have tests when necessary, are there negative and positive tests? Does it pass those…

>The work has already been done. Try to capitalize on it, instead of dismissing it.

Except it hasn't at all. This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted.

The work is integrating that code with the rest of the system, ensuring it's compatible with future plans, ensuring that it's scalable and secure, and ensuring that the whole team can understand and maintain it.

Successful projects don't just take any code that compiles and passes the tests, they are built in a thoughtful manner with high standards.

Post reply on HN