3 ms·

"Keep your PRs as small as possible" In my org we struggle with this because of two combating philosophies -- the first one is the obvious one, "Keep it short

"Keep your PRs as small as possible" In my org we struggle with this because of two combating philosophies -- the first one is the obvious one, "Keep it short and sweet so it's easy to understand and review." The other side is, "Please stop creating PRs and checking in dead code that is never executed until a promised future PR unlocks it." My particular product is very complex (read: messy) and minor changes involve lots of work to maintain back-compat. When we tried the "small PRs", we gained easier reviews... but ended up with so many changes that require a future PR to complete the work, and the future PR never comes for some reason or another. Another problem for reviewers is "Even with the PR description filled out, I don't see the value of this PR unless I see all of it together." If you're reading that you probably scoff, "Just fix your process" or "Tackle your technical debt so your PRs are simpler!" You're not wrong, but if only it were something that could be done in a week and not years :)

sukilot 6y ago
Isn't the solution to that as simple as viewing a chain of PRs together, by diffing one got branch against another?
zimbatm 6y ago
I have two recommendations: Make 20% draft for big PRs. The issue with big PRs is that the overall design is already committed. It's hard to tell the person to redo everything according to a different design. So instead, have the author create a mock PR that outlines the desired changes, and have that being reviewed. This allows to have deeper conversations than if the code is aligned properly. The second thing is; do synchronous reviews for big PRs. Actually take the time to have a 1:1 video call and walk through the code changes. You will be surprised how effective that is, and it helps bond the team together.
bendiksolheim 6y ago
You are alone with this problem. Finding the right size for a PR can be really hard. Too big, and no one really wants to review it because it is too time consuming. Too short, and it fails to show the bigger picture. We struggle with exactly the same thing, with a bias towards too large PRs. I am not even sure I can always agree with myself here. I tend to favor small PRs myself, but I also really dislike PRs that are only part of the solution. PRs should not only be about critiquing the actual code lines, it should also evaluate and discuss the overall architecture, security aspects, performance, readability, tests and probably other things as well. This is difficult to achieve with small PRs. The solution is never to "just fix your process", in my opinion. The correct process depends on the people involved, the culture in the team and organization, the importance of the service in question, among other things. Finding the correct process is a never ending task :)

sukilot 6y ago

Isn't the solution to that as simple as viewing a chain of PRs together, by diffing one got branch against another?

zimbatm 6y ago

I have two recommendations: Make 20% draft for big PRs. The issue with big PRs is that the overall design is already committed. It's hard to tell the person to redo everything according to a different design. So instead, have the author create a mock PR that outlines the desired changes, and have that being reviewed. This allows to have deeper conversations than if the code is aligned properly. The second thing is; do synchronous reviews for big PRs. Actually take the time to have a 1:1 video call and walk through the code changes. You will be surprised how effective that is, and it helps bond the team together.

bendiksolheim 6y ago

You are alone with this problem. Finding the right size for a PR can be really hard. Too big, and no one really wants to review it because it is too time consuming. Too short, and it fails to show the bigger picture. We struggle with exactly the same thing, with a bias towards too large PRs. I am not even sure I can always agree with myself here. I tend to favor small PRs myself, but I also really dislike PRs that are only part of the solution. PRs should not only be about critiquing the actual code lines, it should also evaluate and discuss the overall architecture, security aspects, performance, readability, tests and probably other things as well. This is difficult to achieve with small PRs. The solution is never to "just fix your process", in my opinion. The correct process depends on the people involved, the culture in the team and organization, the importance of the service in question, among other things. Finding the correct process is a never ending task :)