3 ms·
Maybe an obvious one, but at my last org I often wished for more emphasis on keeping patch sets SMALL. I might have liked some kind of soft ban against putting
by ttgurney 4y ago
Maybe an obvious one, but at my last org I often wished for more emphasis on keeping patch sets SMALL. I might have liked some kind of soft ban against putting huge patch sets up for review. I'm talking like 1000+ line stuff. In my experience, no one wants to review those; it's incredibly tedious.
I say "soft" ban because there are exceptions. I don't mind huge patch sets if the changes are proportionately trivial: One example is mechanical search-and-replace jobs on large codebases. But the 3000-line new module that just gets dumped on reviewers all at once is in bad taste. It's the responsibility of the author of the code to ensure that it is broken up in a way that makes reviewers' job tolerable. My opinion.
- icedchai 4y agoThis would happen frequently at a previous job, and it bothered me. Thousands of lines of changes in a repo where you have very little context, not just about the change, but about the project as a whole. While you're trying to learn about the project, submitters are hounding you with Slacks to get the review in so they don't miss some arbitrary sprint deadline. If code reviews are to be done seriously, time should be allocated / scheduled properly (for the reviewer.)
- zevir 4y agoThanks! Do you think it would help if you had a live preview environment that came with every PR, so you could better understand the context of the changes you are reviewing?
- icedchai 4y agoThat would help, definitely! Unfortunately, I think the overhead would be too much for some organizations.
- zevir 4y agoThanks! Great point