> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…
Those architectural decisions should be reviewed during the design and planning phase so mid/low-level devs don't waste time building the wrong thing in the first place.
Is there any tooling you use to squash the commits and use the PR message?
Github has a "squash and merge" option during merging. You can disable the other options (merge, rebase) in the repo settings. I've worked with squash and merge almost everywhere and it's great. Keeps history clean (1 commit = 1 ticket, usually), and links back to the PR with discussions and context.
Unless your tickets are tiny, the single commit will have multiple functional changes and be harder to rollback.
I squash my commits into functionally independent units so they make sense and can be rolled back easily. Rebase FTW.
Have I missed the feedback from the users? There should be some quotes from team members who liked the change. Their mentioning that they start being data-driven for internal tools suggests that they start treating developers like cattle and not pets. >Driving down Time In Review would not only make people more satisfied with their code review process, it would also increase the productivity of every engineer at Meta…
>They have shown that the people they nudge are more likely to do a code review. But are they the experts who do the review well? I think there's assumption that people won't just rubberstamp significant diffs to code they don't own. When submitting a change to another team's project if the reviewers that are suggested aren't actually the right person they are more likely to know the right person who should review it…
> I think there's assumption that people won't just rubberstamp significant diffs to code they don't own
I wouldn't advise doing that but to play devil's advocate, why shouldn't they do that when number of reviews are probably a metric in their performance review? What's in place to discourage that?
> Next reviewable diff Holy Fuck No. 90% of my PR review time goes into "Okay, how will this change impact parts of the system that this mid-level Dev doesn't understand yet?" and "Does this PR actually do what the ticket it is claiming to implement actually intended?" Reviewing diffs in isolation completely removes one's ability to do that. If you remove a person's ability to do that, what you've left them with is t…
> "Does this PR actually do what the ticket it is claiming to implement actually intended?" Let me ask about your unspoken assumption: is this the PR reviewer's job? Maybe the PR seems to implement what the ticket asked for, then after merging it becomes clear that it didn't fully implement it, or the business stakeholders are unsatisfied, etc.?
It's not the PR reviewer's job to go actively test it out (you can assume that your colleagues are somewhat competent at what they're doing), but if you review with the spec or the issue open on the side that says to add a blue button and you see it's red, it's your job to ensure it's not a mistake and point it out to the author.
Am I reading this right? If the total median time in review is “a few hours”, that means that people are dropping what they’re doing to review the code. That can’t really be a good code review? A context switch alone would be half of that time for me.
Having thousands of engineers tends to help on that. If there's a 5% chance one engineer is available in the next two hours after you post a PR, with a team of 10 you will probably have none, with a team of 1000 you will easily get some.
I came to the conclusion long ago that mandatory code reviews are a waste of time. For critical stuff, absolutely. But PRs and review cycles over burden dev teams and don’t seem to move the quality needle higher one bit. A better way is to ensure multiple hands touch a given area of the code, so that multiple eyes ultimately are seeing and manipulating those bits. If they are given a task to do in that area they will…
you'll have compliance needs to speak for if you let code to prod unreviewed
Too many think code reviews are an opportunity for endless debates over personal preference. A code review should be fast and cover blatant good practice violations and architectural mistakes. Everything else should be taken care of by linters and tools. If a reviewer wants code done in a different way they can write the code themselves.
One thing I've enjoyed where I am now is that PR comments come in two flavors. The first, actual feedback. The second, borderline pedantic issues that are prefaced with "nit: " in the comment. Nit comments are safely ignored but are there so that if the author wants to put in that change while changing some other issue, then OK.
the reviewer should make 'nit' changes directly rather than pass a mini-ticket for evaluation back to the author. why are reviewers so scared to modify prs at orgs?
I don't understand this criticism. How does a "next reviewable diff" pop-up suggest that you're being forced to review a diff "in isolation"? As I understand it, nothing proposed in this article prevents you from reviewing the diff in context of the larger piece of software, as you would have always done. This just seems like a feature to suggest another diff for you to review after you've finished accepting/rejectin…
It encourages looking quickly at small bits of changes in isolation as opposed taking a holistic view of the entire change, and viewing them in series as 5 min little tasks you can tick off like tiktok videos. The review tools on github already go too far toward this by removing far too much context.
I work at a company with a similar code review culture. I have about 2 blocks of meetings per day on average. I usually check my assigned code reviews in the morning and when I come back from the meetings. Once I’m done with those then I move on to my own work. If I have too much code review to the point that I can’t get my own work done, I take my name off the reviewer list and it’s reassigned. On most days I’m stil…
> I take my name off the reviewer list and it’s reassigned. How big is your team? We've got 4 developers on our team (and one lead), and require two signoffs for code changes. There's just not enough of us for something like that to work... right?
So every change needs to be seen by 3 out of 5 people (assuming the lead also does code review)? That sounds a bit inefficient.