This document captures how I'd like to pull requests to work. For context, I work on an open source project that has many repos and more importantly a lot of people working on it (in the hundreds). Also, I'm not a developer but a program manager so many of my PRs tend to be non-code, like blog posts and specifications.
However, I don't think my issues with GitHub's PR flow are a function of my role and mostly a function of the team size and thus the number of reviewers I usually have. Many of my peers (who are devs) complain about similar things, which indicates to me that this gut feel isn't entirely off.
Here is what it usually looks like for me:
- Around 10 reviewers (and up)
- Around 50 comments (and up)
Examples:
GitHub provides no facilities to manage this load. At Microsoft, we have an internal tool that works much better. The key features for me are:
-
Comments are threaded. It's simple Facebook-style threading (i.e. a root comment and all children are flat).
-
Root comments have a status. The root comment has a status (active, resolved, won't fix, by design, closed). This allows reviewers and reviewee to quickly assess what work is left to do and what the status is.
-
Comments can be filtered by status and person. This allows both sides to to focus on the particular task at hand.
-
No size limitations. The web UI doesn't work for larger changes or when more context for a hunk is necessary. Granted, I don't usually suffer from this but for real world code changes, touching 50-100 files isn't unusual when a refactoring or a non-trivial bug fix is performed.
Here is how I usually work:
- I filter all comments to the ones that are still active.
- I focus on a given person and work through all their comments.
- I push a single commit for that person, such as "Address Paul's feedback"
- This allows reviewers to see all the changes I have done to address their feedback and assess whether they are happy with the outcome.
- Rinse and repeat until zero threads are left with active.
So like not to sound like a fanboy but VSTS has all of this baked in.