3 ms·

Well then why does the PR template used at the company require manual validation steps on every PR if it's not expected that reviewers do them? Do you really th

Well then why does the PR template used at the company require manual validation steps on every PR if it's not expected that reviewers do them? Do you really think 5 seconds was enough time to "sanity check" the PR? Do you think 5 seconds was enough time to even click the code tab? Why are two "reviews" even required if the reviewers are actually expected to simply rubber stamp every single PR that comes through? Edit: "the fix for a bad PR is more PRs" has got the be the worst take I've seen in years.

xyzzy123 2y ago
The review requirement often comes from compliance, many standards require a control to stop developers getting code into production by themselves without anyone else looking at it. Sometimes it's because an engineering manager read a book that said it was "best practice" or because "that's what we did at $lastjob". Sometimes it's because someone set up the SCM to require it and never looked at it again. Maybe it made sense to someone 5 years ago but it doesn't make sense now, for the teams and processes you have. It's worth asking and finding out! I agree 5 seconds is not enough for even cursory review, why even bother, unless it's a compliance thing. In a functional team the thing you do is raise this in retro and discuss what everyone expects and wants from reviewers.
- reviewmoreprs 2y ago
  If your take is that blindly approving merges with 0 care is actually a good thing and that I "lack understanding" then I simply have to disagree lol. Good luck with that.
  - xyzzy123 2y ago
    Sorry that I was not clear, my bad, I did not mean to imply that you personally "lack understanding" in any way. What I should have written was that there seemed to be a lack of shared understanding among the team, (i.e, agreement) on the value, meaning and expected process for PR reviews. I don't think there is any particular level of care that makes sense in all cases for PR reviews, I believe it depends on industry, criticality, the particular repo and who is doing what. I think the most important thing is that everyone is on the same page about what's expected.

xyzzy123 2y ago

The review requirement often comes from compliance, many standards require a control to stop developers getting code into production by themselves without anyone else looking at it. Sometimes it's because an engineering manager read a book that said it was "best practice" or because "that's what we did at $lastjob". Sometimes it's because someone set up the SCM to require it and never looked at it again. Maybe it made sense to someone 5 years ago but it doesn't make sense now, for the teams and processes you have. It's worth asking and finding out! I agree 5 seconds is not enough for even cursory review, why even bother, unless it's a compliance thing. In a functional team the thing you do is raise this in retro and discuss what everyone expects and wants from reviewers.

reviewmoreprs 2y ago
If your take is that blindly approving merges with 0 care is actually a good thing and that I "lack understanding" then I simply have to disagree lol. Good luck with that.
- xyzzy123 2y ago
  Sorry that I was not clear, my bad, I did not mean to imply that you personally "lack understanding" in any way. What I should have written was that there seemed to be a lack of shared understanding among the team, (i.e, agreement) on the value, meaning and expected process for PR reviews. I don't think there is any particular level of care that makes sense in all cases for PR reviews, I believe it depends on industry, criticality, the particular repo and who is doing what. I think the most important thing is that everyone is on the same page about what's expected.