4 ms·
We have a rule internally that a PR can’t be more than 1,000 loc. I personally find a 1,000 loc PR almost unreviewable. It takes me multiple days and a stack of
by donatj 5y ago
We have a rule internally that a PR can’t be more than 1,000 loc. I personally find a 1,000 loc PR almost unreviewable. It takes me multiple days and a stack of notes, and still almost always ends up with effects I didn’t anticipate.
Trying to understand the changes and their full implications is nearly impossible. 100 loc is a better top end imho.
- dlsa 5y agoCode reviews are interesting to watch. Especially when the author awkwardly pauses and says, "I'm not sure what this code does".
- coldcode 5y agoLast place I worked (big company, not FAANG) I remember a PR for our main app that affected 500 files and occasionally dozens of places in some of them. All the reviews were "LGTM".
- ascagnel_ 5y agoI forget where I first saw it, but there's a joke of "ten lines, ten issues; 1,000 lines, looks good". There's a human limit to how much you can hold in your head when reviewing code.
- whimsicalism 5y agoI think it's a double edged sword - people are also way too nitty for small commits, which only incentivizes behavior like this.
- thetallstick 5y agoLooks like the programmer's version of: "If you owe the bank $100, that's your problem. If you owe the bank $100 million, that's the bank's problem.” https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-100-that-s-your-problem-if https://www.goodreads.com/quotes/214064-if-you-owe-the-bank-...
- sockpuppet69 5y ago
- doix 5y agoWhen you refactor a commonly used structure, it's easily possible to hit situations that you described. In those cases, having nicely organized commits are super important. You need to make sure that the commit that does the refactor only does the refactoring. Nothing else, then you can review the PR commit by commit and it becomes much more manageable. When people start doing multiple things in a single commit with 1000's of changes, it does become mentally exhausting to review. Unfortunately, it requires either being extremely diligent when making the commits or being familiar with git rebase -i. Of course, sometimes it's just thousands of lines of new code and impossible to review :).
- lulzury 5y ago10 lines of code = 10 issues. 500 lines of code = "looks fine."
- travisgriggs 5y agoParkinson’s Law of Triviality
- Cullinet 5y agoabout days and a stack of notes : this is why we have line printers, custom reading tables with balanced soft lighting (print proofing lamps) and more recently, rostrum cameras operated by foot switch. review with fanfold might be a habit only acquired by formative experience, but once you physically manage the printout, and are comfortably seated with several hundred well spaced lines that aren't directly emitting typically exhausting spikey spectrum light forcing your brain to perform extremely complex colour management to balance your vision, which is the primary factor in concentration depletion in every attempt at testing we've tried over the years, and you mix in modern handwriting recognition and a pico projector display of feedback comments if necessary, I have only witnessed very high productivity. Anyone who is subject to audits will know the satisfaction of dumping fanfold boxes on the auditors desk. Or should.