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

291–300 of 302 posts

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

#291

Post by hyggetrold »

MarkSweep wrote: post #272
hyggetrold wrote: post #90

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

Stacked pull requests are often a solution to, not that cause of, untold suffering. They let you put blinders on your reviewers to they don’t startle about unrelated changes.

> Stacked pull requests are often a solution to, not that cause of, untold suffering.

I understand what you mean by this, but in my mind it's a poor solution because it doesn't fix the root cause. The root cause of stacked PRs (in my experience) is that people are waiting so long for review that they branch off the initial change.

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

#292

Post by withinboredom »

Arainach wrote: post #289
withinboredom wrote: post #277

Earlier quoted context omitted.

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

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

To be honest, I’m not sure how you got there from here…

> 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.

I actually suggested deprecating the library. I am not going to presume how hard that is. It might be changing 2 lines of code or 1000. No idea. That’s why we have brains though, we are able to figure these kinds of things out. As far as ensuring it’s compatible with future plans… do you honestly think a “random” maintainer of a library has your best interest at heart? That’s rather unlikely unless the maintainer happens to work for the company in question. In this particular case, it looks like someone volunteered to be that maintainer (probably due to naivety, but that’s beside the point) and they actually do have your future interests at heart.

So to say that an employee doesn’t do work aligning with their company’s goals means you have bigger (political) problems than code review problems, hence why I said they should never have started working on it in the first place. For them to work on something like this for someone to review and professionally say “wtf” is a red flag, far and beyond code reviews; which is interestingly another reason why it is waaay too late to be having these conversations in a PR. It’s a smell that something in the team is fundamentally broken.

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

#293

Post by Arainach »

withinboredom wrote: post #292
Arainach wrote: post #289

Earlier quoted context omitted.

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

> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > 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. I actually suggested deprecati…

>To be honest, I’m not sure how you got there from here…

"I've done all this work, get put of my way and submit" is the attitude in both cases.

Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with.

The default answer to code review feedback is always "sure" unless you have a strong argument otherwise. It takes way less time to implement feedback than to pretend you're Socrates and question everything.

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

#294

Post by spartanatreyu »

mstachowiak wrote: post #259
spartanatreyu wrote: post #254

Earlier quoted context omitted.

Something is going horribly wrong on this page. Putting an svg filter over the video element is making the page render 1fps a second on Firefox MacOS. I legitimately thought you uploaded a massive gif as your feature asset instead of a video. If the effect is always on the video, you may want to just bake it into the video. Otherwise, you might want to recreate the filter without using SVGs to do it.

Thanks for highlighting, we're investigating and should have a fix shortly.

Looks like you fixed it!

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

#296

Post by SAI_Peregrinus »

dgellow wrote: post #217
yegle wrote: post #152

I don't see it mentioned, but there's a recent addition that greatly improved my turnaround time to review incoming CLs: A prompt will show up in the corner about the next pending CL that you can approve, when you give LGTM to the CL that you are reviewing. It's kind of like those "Customers who bought this also bought" prompts on e-commerce websites. Only here it slightly nudges you to have a "CL review streak" and…

I never thought about this but it’s a fantastic idea. Would love to have something like this for GitHub.

GitHub Notifications sort of does this, though it's not just for PRs where you've had review requested (it's gets noisy because it also includes things like PRs merging or being closed).

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

#297

Post by withinboredom »

Arainach wrote: post #293
withinboredom wrote: post #292

Earlier quoted context omitted.

> This is the attitude of people who send pull requests to open-source projects and complain that they aren't accepted. To be honest, I’m not sure how you got there from here… > 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. I actually suggested deprecati…

>To be honest, I’m not sure how you got there from here… "I've done all this work, get put of my way and submit" is the attitude in both cases. Congratulations, you wrote some code. It's not done until it meets team/project standards, and getting offended any time anyone has feedback makes you someone I'd never want to work with. The default answer to code review feedback is always "sure" unless you have a strong arg…

> "I've done all this work, get put of my way and submit" is the attitude in both cases.

That’s quite a stretch. Nobody should be “in your way” to begin with. In a company, you are on a team and the point of a team is to work together; not get in each other’s way. That’s a sign of a dysfunctional team, which is how you end up getting team members working on the wrong thing or implementing things wrongly in the first place. Code not up to “standards” is not a reason to reject code, it’s a sign that something else is wrong on the team and that is the thing to be addressed, not the code. You aren’t going to resolve it in a code review.

I don’t know how else to say that at this point.

> The default answer to code review feedback is always "sure" unless you have a strong argument otherwise.

I’ve definitely pointed out why a deleted line needs to be kept. I would not be surprised if that someone came back to tell me why it should be deleted. As a reviewer, I lack a lot of context even if I originally wrote the code. I’d never, ever, expect someone to blindly say “sure” on a code review. I don’t write perfect code and neither do my team members, so I don’t expect a perfect reviewer.

A team works together to make the code perfect, nobody is writing mystery code and doing weird stuff without discussing it with the team first. And sure, people experiment and we review it, but with an architectural lens. Then when we are happy with the architecture, we actually implement it, together.

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

#298

Post by tgsovlerkhgsel »

SpaceManNabs wrote: post #146
tgsovlerkhgsel wrote: post #80

Earlier quoted context omitted.

Bazel is a build system, not a code review system. Entirely different thing.

You should open the article. The very first content in the article is someone praising critique and also saying they wont use bazel.

Thanks - missed that because it was in a picture. I even went back and CTRL-F'd before I replied but of course that didn't work either...

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

#300

Post by redditor98654 »

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…

> Suggesting a library should have been done before the code was even written.

Unless you agree on every detail before implementing anything, I don't see how this is practical. At least not in the companies I have worked in.

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

Not quite though. The developer may have written down some code but the work is not done. It needs to be shipped to production, monitored and supported by the oncall etc. If there is a way to not write some code and use a library, I will suggest that.

> Don't be an ass and call them out for it on the PR in front of the whole team, give them the benefit of the doubt and if someone else comes along and calls them out, you can present a united front vs. forcing them to defend themselves all alone.

If I see a PR with some key tests missing, I don't see why asking if we can add some more tests would be seen as calling them out on it. The PR is a place to record such things - may be it does not need such tests or may be it is intentionally not handled in the code etc. Why would such a discussion be seen as someone being an ass?

Why do I have to have that perfectly normal discussion in secret away from the rest of the team? We can ask questions and still be professional. Code reviews are not just for making sure code is good, but also for education - it is a good way for the other to know what is going on and also learn.

My read on this is that may be this is what happened to you - someone was being personal and attacked team members personally in the guise of a review and now you take a stance that either discuss everything before implementation in person or have separate meetings in private to suggest changes or ask questions.

I am thankful I did not have such a colleague/mentor when I started and hopefully I am not inflicting such an attitude to the new folks that are coming in now.

Post reply on HN