Comment by sarchertech 5 days ago
Sure that’s fine when you’re the one doing the review for a newcomer or less experienced dev.
However, I think that PR reviews after the code has been written are the absolute worst time to do this.
I'm not saying I never fire up a draft PR so that I can nail some things down and get comments on it.
But at the same time not everything needs to be asynchronous and if you try to make everything asynchronous, such that you never have ad hoc meetings or synchronous conversations, you're going to move much slower than is necessary. I've been exclusively remote for 10 years now, so I love doing thing async when I can, but I recognize the limits.
By the time my PR is ready to merge the assumption should be that it's ready to merge, unless you see something actually wrong. Not just hey this could be a little easier to reason about if you completely redesigned your solution.
9/10 when people do make those kinds of suggestions it's not objectively better, and 9/10 changing it won't move the needle with respect to maintainability or meaningful performance.
So given that we have a funnel of reviewers where most people are only going to do surface level reviews, of those that do deep dives most of them aren't going to find any problems, of those that do most of the solutions aren't going to make any kind of significant impact. Is it worth going through this whole process? My conclusion is that no it is not.
Comparing the software we produced before the whole PR review for every merge request thing became common, and the software we produce now, I don't think software quality has improved at all. So for me to change my mind, I'm going to need to see hard data that says the opposite.
> 9/10 when people do make those kinds of suggestions it's not objectively better, and 9/10 changing it won't move the needle with respect to maintainability or meaningful performance.
The 9/10 number seems incongruent with my experience, but that could be because I often help set the PR review culture, and encourage people to identify the questions: is it "how" (…is it working), "what" (…is it doing), or "why" (…is it done this way). If you recognize those, it makes it way easier for the author to address the specifics without a total redesign. (How = hard to follow the logic, what = better names needed, why = comments needed).
But sometimes a redesign can be super compelling to everyone, and the question is: do we want to be stuck with the subpar version? Because really, the closer you are to remembering how it all works, the easier it is to rewrite it. Not later, when it faded from memory and overgrew with dependent code.
> Comparing the software we produced before the whole PR review for every merge request thing became common, and the software we produce now, I don't think software quality has improved at all. So for me to change my mind, I'm going to need to see hard data that says the opposite.
The kind of subjective reviews we're discussing (not security, performance, consistency, edge case issues, or major simplifications) are not really about quality, they're about your specific team understanding your specific code. Help your colleagues (just the small circle of maintainers) have more confidence in coming in later to change this code. If you're working alone, this isn't as big an issue.
To collect data on this, you would need to witness what happens when everyone architects their own way, and then you have to find the person who wrote that part to decipher it, and compare the amount of slowdown that can cause. Nearly impossible to measure in practice, I think.
> not everything needs to be asynchronous
I completely agree. PRs provide great tools for discussing code asynchronously so it's good to lean on them sometimes, and discuss the work on a call other times. This is however a big departure from your original statement that PRs are the absolute worst time to review the code.
> but that could be because I often help set the PR review culture
I highly doubt you'd be able to do this outside of a small company, or a small self-contained team.
I've seen those exact questions in PR templates before, but they almost always end up ignored or answered in the most obvious surface level manner. I see no reason you'd be able to reliably get reviewers to do any better. Really understanding a PR takes time, and the time to do that is one of the first things that gets jettisoned when anyone is under any kind of pressure to deliver.
>But sometimes a redesign can be super compelling to everyone, and the question is: do we want to be stuck with the subpar version?
If it's really going to make a difference sure. The point is is this the case frequently enough for the process to have a positive ROI. I don't think it is.
>are not really about quality
If they aren't improving quality, then they better be improving velocity. I highly doubt this is the case. I've never personally seen it. Given the enormous cost, I want hard evidence to justify that cost.
>when everyone architects their own way,
That's not the alternative, before mandatory PR reviews, we still had, design reviews, white board sessions, code reviews, pairing sessions, planning meetings etc...
>This is however a big departure from your original statement that PRs are the absolute worst time to review the code.
Catching a problem just before merge after someone has written everything they plan to merge is self-evidently the worst time to catch a problem (next to after it has been merged of course). Now you can argue that it may be the most likely time to catch a problem, but there's no arguing that it wouldn't have been better if the problem had been caught earlier.
My argument is that in addition to be the worst time to catch the problem, it's not actually catching real problems enough to be useful.
> I've seen those exact questions in PR templates […] understanding a PR takes time […] when anyone is under any kind of pressure to deliver. […] is this the case frequently enough for the process to have a positive ROI.
Not templates. The culture HAS TO be for everyone to calm down and treat PR reviews as first class citizens, not unwanted distractions. If ya'll stop stressing each other out that reviews are bs, I'm sure they'll stop being that.
> […] we still had, design reviews, white board sessions, code reviews, pairing sessions, planning meetings etc...
You know that feeling when you get a design idea, you start implementing, and a single plot twist makes you realize it doesn't fit? What do you think happens when a white board session bumps against reality? That's likely a session down the drain.
What if we replace it with the following process: 1) Discover all the plot twists up front by writing out the solution. Publish a PR. 2) When a reviewer comes in, give them time to grok it, welcome redesign ideas. 3) Easily assess and validate their ideas using the stupid amounts of expertise you temporarily gained by having written a working solution. 4) Discuss or proceed with fixups.
How can it get more concrete and grounded in reality than this? One of you knows every tree, the other one can still see the forest. I'd argue, you rarely need to talk before this point. This is the best point.
(Obviously you shouldn't just suffer in silence if you're stuck or your diff is getting out of control. Run it by somebody sooner than later.)
> I highly doubt you'd be able to do this outside of a small company, or a small self-contained team.
Thankfully most companies or teams are indeed small.
I completely disagree. By writing the code, the author gains understanding of the hidden details of the problem. The reviewer gets to explore a full solution that accounts for those details, and provides tests. With the context still fresh, it's a great time for a new pair of eyes to explore, ask questions (which should lead to clarifications in the code), and think of ways to simplify and untangle the solution further while keeping the tests passing. In fact, it can take more time to keep assuming and discussing what the solution might be than to write the code and iterate on it. In an asynchronous/remote work environment, iterating on PRs is not only okay, but probably the most effective way to collaborate.