4 ms·

>In a nutshell, you know as well as I do that if someone sends you a PR with thousands of changed lines and dozens of changed files, you’re more likely to “LGTM

>In a nutshell, you know as well as I do that if someone sends you a PR with thousands of changed lines and dozens of changed files, you’re more likely to “LGTM” than spend hours trying to get through it. I think he's right that this is a common attitude, but that attitude drives me nuts. If someone sends me a huge PR with thousands of lines, I definitely do not just blindly approve it and would be furious if that was happening on my team. If the PR is too big to review, then push back and insist on splitting it up into more manageable pieces.[0] Sometimes PRs have to be enormous, but that should happen rarely on a normal codebase. [0] https://mtlynch.io/human-code-reviews-2/#look-for-opportunities-to-split-up-large-reviews https://mtlynch.io/human-code-reviews-2/#look-for-opportunit...

schacon 2y ago
I think it's more interesting to build tooling that helps with the core problem rather than try to change people's attitudes. The problem imo is not that people are lazy, but that the review tooling is not good at the problem set of large changes that need to be introduced atomically. Even if a change is not enormous, it can still be difficult to grok the context of a change when it's all squished together. Reviewing a series of smaller changes with good context messages is just mentally easier and technically simple (plus ends up with better commit messages for future context). I've seen Butler Requests come in via our internal usage where the unified diff is still fairly small, but it's much easier still to review it properly when it's broken up into semantically grouped changes with good commit messages.
- mtlynch 2y ago
  Hi, Scott! Thanks for your response. To be clear, I like the work you're doing. I definitely agree that the unit of review shouldn't be one commit at a time or every commit all at once, and I'm happy to see you invest in tooling that offers a better path. >I think it's more interesting to build tooling that helps with the core problem rather than try to change people's attitudes. The problem imo is not that people are lazy, but that the review tooling is not good at the problem set of large changes that need to be introduced atomically. I think it's both, but I think mindset shift has to precede tooling in this case. If the team/org culture tolerates people just blindly LGTM'ing large commits, that's a culture problem at its core. Even if they have better tooling, what's motivating people to invest extra work to break up changes into small, logical chunks rather than continuing to just throw huge diffs over the wall if those get LGTM'ed with no pushback?

schacon 2y ago

I think it's more interesting to build tooling that helps with the core problem rather than try to change people's attitudes. The problem imo is not that people are lazy, but that the review tooling is not good at the problem set of large changes that need to be introduced atomically. Even if a change is not enormous, it can still be difficult to grok the context of a change when it's all squished together. Reviewing a series of smaller changes with good context messages is just mentally easier and technically simple (plus ends up with better commit messages for future context). I've seen Butler Requests come in via our internal usage where the unified diff is still fairly small, but it's much easier still to review it properly when it's broken up into semantically grouped changes with good commit messages.

mtlynch 2y ago
Hi, Scott! Thanks for your response. To be clear, I like the work you're doing. I definitely agree that the unit of review shouldn't be one commit at a time or every commit all at once, and I'm happy to see you invest in tooling that offers a better path. >I think it's more interesting to build tooling that helps with the core problem rather than try to change people's attitudes. The problem imo is not that people are lazy, but that the review tooling is not good at the problem set of large changes that need to be introduced atomically. I think it's both, but I think mindset shift has to precede tooling in this case. If the team/org culture tolerates people just blindly LGTM'ing large commits, that's a culture problem at its core. Even if they have better tooling, what's motivating people to invest extra work to break up changes into small, logical chunks rather than continuing to just throw huge diffs over the wall if those get LGTM'ed with no pushback?