Comment by sarchertech
Comment by sarchertech 3 days ago
> 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.