"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.

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.

I don't see how taking it file by file lowers the complexity. If a file introduces a new method and that method it used 10 times in different files throughout the PR, looking at the 1 file doesn't really tell me much. This is compounded when those other 10 files have other changes unrelated to whatever new function I'm trying to review.

We have good unit test coverage and a decent deployment process so I'm rarely concerned about something seriously breaking, its more trying to answer the question "is this approach reasonable" or "could this be done more simply" that I find really hard to answer when there are so many changes bundled together.

Then, yes, if the risk is largely off loaded to automation tasks then larger PRs become far more reasonable. Reviews file by file does not lower complexity, but just segments your effort so as to allow the task to become less daunting. Since you do have good automation in place perhaps the most important thing to review during your PRs is organizational complexity, which appears to be your primary concern. If the change requires tremendous effort due to organization concerns that might be a strong point for push back and refactor such that like conventions are grouped together to lower complexity into the future. Although refactors like this increase effort in the immediacy they lower tech debt at future maintenance. Another benefit of strong automation already in place is that large scale refactors are low cost efforts.

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

It wouldn't matter in this case. Its usually 1 1500 line commit in a PR, or its a couple of commits but they aren't meaningful ("write large feature", "review updates", "wip", "merge branch X", etc)

Why are people writing junk commits like that? Tell them not to

Some people consider the PR the unit of work, not the commit. I don't really agree but I think its a reasonable way to do things if you are squashing the PR before merging. The problem is the size.

> Please tell me you guys aren’t squashing commits on merge too

We (different org) aren't. We really should, huh?

No you really shouldn’t squash commits such that each one is hundreds of lines. That makes future debugging with git bisect pretty difficult

On the other hand, on a larger scale, it's often more about figuring out what PR caused something to break, rather than exactly which of the 100 WIP commits inside that PR was the exact cause.