What is wrong with bazel?
A study of Google's code review tooling (Critique)
31–40 of 302 posts
Re: A study of Google's code review tooling (Critique)
#32> 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…
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 that you're in the top x% and lucky to be there. People generally don't really feel the need to crap on people in code review to make themselves feel self-important.
Re: A study of Google's code review tooling (Critique)
#33Shameless but relevant plug: I built CodeApprove (https://codeapprove.com) to bring the best parts of the Critique workflow to GitHub PRs. Obviously there are many things that don't translate (Google doesn't use git, and it has insane CI/CD integration) but CodeApprove gives you the same workflow. You always know which conversations are resolved, which PRs need your attention, etc.
Feel free to reach out via email if you're curious.
Re: A study of Google's code review tooling (Critique)
#34> 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 think PRs are necessary. You can make a very minor change that introduces a bug or discrepancy in documentation. But I 100% agree with your "get out of the way" philosophy. That notice should come with the PR notification for the reviewer!
Re: A study of Google's code review tooling (Critique)
#35Earlier 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
It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
Re: A study of Google's code review tooling (Critique)
#36> 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 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 my experience they say, "ah, nice, thanks, I didn't think of that / I wasn't aware of that, I like that more; updated now", and then everybody is even happier.
Another useful thing, in my opinion, is to use your "fresh eyes" to make suggestions on what could use to be clarified in code or documentation. "This wasn't immediately obvious to me, could you add a comment on why it works this way?". The author is often too close to the code to see that it isn't obvious, because it is to them, because they've already spent hours thinking about it. But this should almost always what be optional suggestions. (But, rarely, code is so unclear that it really shouldn't go in without being clarified or documented.)
But in general I do think it's important to consistently be thinking to oneself "my goal is to be useful to my team and organization. are my comments achieving that? would it be more useful to approve or to not approve this right now?". The temptation to be a fastidious editor can be strong - if most of us didn't have this impulse we'd probably be doing different jobs... - but I find this "usefulness" framing to help me a lot in resisting it.
And very strong disagree on not requiring reviews. It is much better to tirelessly foster a low-friction review culture.
Re: A study of Google's code review tooling (Critique)
#37> 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…
> 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 written it, get out of the way. If you're saying anything that isn't directly related to a necessary change to the code, get out of the way.
But how will I prove my 'mentorship' abilities and seniority at FAANG without pretending my subjective nitpicks and lack of understanding are critical PR blockers? /s
Re: A study of Google's code review tooling (Critique)
#38> 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…
Hear hear! Nit picking can be soul crushing. Having to argue with someone over something which is trivial and does not matter is a sure fire way to kill my productivity.
Re: A study of Google's code review tooling (Critique)
#39> 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 am as unopinionated as I want others to be. All they need to do is commit a patch to my branch and fix it. It won't hurt my feelings and I don't need to consent. Let's get past the egos and wasted time asking for permission to change things.
Re: A study of Google's code review tooling (Critique)
#40> 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…
100%, extra points for fitted penname.