It depends on many factors. No policy fits every team or situation. If feedback times - weather due to pipeline or personal/reviews - are long, I would also push larger PRs. Also reviews rarely really improve anything besides bikeshedding stuff like style or whatever the reviewer might find more pleasant in terms of overall structure. If reviews were done right you’d have to deep dive into the problem the requester tried to solve and understand his concepts and ideas of the solution. To do so, I do important reviews (which are super rare) in pair programming style with the requester. I suggest you should do the same especially if lot of code is changed or added. I assume you have proper testing in place. Looking at tests can help a lot to understand production code
That depends upon how your organization manages branches, releases, and such.
Just take it file by file. If you are using something GitHub Enterprise or the Bitbucket or GitLab equivalents then just leave comments file by file of your progress in reviewing the PR.
Is there a parallel stream for corresponding changes to test automation? If so that helps tremendously. Are there rigorous automated lint/validation/conformance rules in place, because if so that helps tremendously. With enough automation in place your review should be a quick look line by line for major violations and questions about why some change was needed from the business requirements perspective as answered by code comments or documentation. 1500 line PRs don't have to be painful if the platform is mature.
You should bring this up during your retrospectives, or at least to the dev whose code you're reviewing, your team lead, and your manager if you don't do retros
If the PR is too large, chances are that the task was too big and should have been broken down further. If this happens regularly, your team needs to reevaluate your approach to working on tasks and see how you can organize and ship smaller chunks of code
Yes 1500 line PRs that are anything other than trivial changes are not acceptable. Please tell me you guys aren’t squashing commits on merge too
"Acceptable" depends on the team and organization norms and processes, not on the pull request.
Reading your post I thought about how often programmers talk about code "readability" and "maintainability," how we should all write our code in a way to make it easy for the next programmer to read and understand. And then I see posts like this, complaining about having to read code and understand and learn from it as part of the job.
If you view your job as a programmer as mainly writing more code rather than producing business value you will worry about things like the size of pull requests, number of meetings, learning new tools, even the waste of workplance social interaction. Any time spent not writing code gets devalued and treated like a distracting annoyance.
Reading code other people on the team wrote potentially has a lot of value. You can learn more about the overall code base beyond the parts you work on or care about. You might see novel techniques and approaches to solving problems. You might find opportunities to ask questions and collaborate (another thing programmers claim to value but actually don't do that often). Most important, reviewing, understanding, and maybe testing the work of other programmers, in the form of a pull request, directly contributes to the entire project/product in terms of business value, the value that actually matters much more than the isolated silos of code that programmers like to focus on.
I sigh when I see a big pull request too, but I remind myself that reviewing and understanding that work ultimately adds more value to the product than any code I will likely write in the time the PR review will take. I will just have less fun and derive less ego gratification from the PR review.