Plan to get our PR queue to a 'to-do list'
Reviewing contributed work
Reviewing contributions essentially means looking at the PR queue. As at writing there are 149 pull requests in the queue. This probably equates to upwards of 300 hours of work that has already been done and upwards of 300 hours of work to get them all reviewed. Most of the work done was probably funded but perhaps as little as 0 of the hours to get them reviewed is funded.
This resource gap is not easily solved so we need to think about efficiency within it. A specific issue is that it can take as long to find a PR that is reviewable in a queue with ~150 PRs as to actually review them.
Here are some principles I propose
- Recognise our reviewer resources are our least funded and scarcest resources. We need to be respectful of them.
- Not only is review work generally unfunded, it is time consuming and it involves taking on responsibility for a change that is not to their benefit but they will sure have to ensure is fixed if it causes a problem, and potentially bear the brunt of any criticism from the community.
- Reviewers will make mistakes and we need to accept that in a way that does not reduce the number of people prepared to do review.
- The PR queue should be a place for PRs currently active or fully reviewable.
- PRs suck up time just by existing. There are a number of PRs in the q which I have spent maybe 6 minutes looking at & concluded that I wouldn’t review it because I would never merge it without a test & there is not one. The problem is I might have spent those 6 minutes 20 times over the lifetime of a PR
- A PR does not have to be open for discussion to take place or for it to be accessible later.
- Where a PR cannot be resolved easily we should consider the primary place to track it as being gitlab & ensure it is linked to an issue there. If the PR is closed it & the discussion on it can be found from gitlab.
- We should close PRs that are not ready for review
- if we look at a PR & think ‘I wouldn’t merge this without a unit test’ we should make that comment on the PR and close the PR with a request to re-open in when there is a test
- If the PR is not truly recent & has become a discussion piece we should close it with a request that it is re-opened when the discussion has resolved.
- If a PR is stale we should try close it & ask for it to be re-opened
- If the quality of code is not really there we should consider closing & doing any mentorship offline
- If the approach is not right we should consider closing & requesting a new PR with a new approach
- If a PR is truly active we should hold off (new PRs often generate discussion & we should encourage that, but once they are no-longer near the top of the list ad hoc discussion becomes unlikely)
- The PR queue is nobody else’s to-do list. Sometimes people track their own work through their own open PRs. That’s not a legitimate use of the PR queue.
- WIP Prs should only stay open for a short period
- We should be careful about reviewers writing tests for other people’s PRs (or taking on fixing the bug if the PR is not up to scratch)
- It has been frequent for reviewers to write tests for other people’s PRs. If someone submits a bug fix that we would not merge without a test we have often written them. However we need to recognise that we are prioritising limited resources and consider closing with a suggestion to re-open when a test is present unless other factors. (There are numerous reasons why a reviewer might still choose to write a test but they should at least feel the option is theirs to request a test without having to leave the PR in the queue)
-
We should identify & prioritise PRs that have unit tests
-
We should identify & prioritise PRs that fix bugs
-
We should identify & prioritise PRs where ‘most’ of the work has been done
- In general by the time a PR has been submitted somewhere between 10% & 90% of the work on that bug has been done. Try to identify & focus on those closer to 90% & close those closer to 10%.
- We should identify & prioritise PRs by people who act as reviewers themselves
- This is out of respect for the work that reviewers do, and in recognition of the fact that a lot of their PRs are in response to that work rather than their own paid work anyway.
-
We should identify & prioritise PRs that have had endorsement from community members In particular this should be when the endorsement says something like ‘I’ve tested this & it works’ as they have done part of the job of the PR already.
-
We should identify & prioritise PRs by people who have a track record of engaging in review & treating reviewers with respect
- It is not infrequent for a reviewer to find that the PR submitter feels that by submitting the PR they have done their bit and to not wish to put any more time into the PR after that point. This places an implicit value on their time and none on the reviewer’s time. However, the majority of submitters DO appreciate the time reviewers put into doing review. It is valid for reviewers to focus their efforts based on return on their effort. (Note I think any discussion based on this should focus on ‘reviewing Matt’s PR would be a good idea since he is always really responsive. I’m sure most reviewers have a mental list of people they have been stung by, but discussing those is not helpful). In the bigger picture we need to educate people that submitting a patch for review is not job done but job started.
-
We should try to improve our review soft skills We often see PRs that have had a lot of discussion, sometimes on trivial details, without big picture review. In some cases reviewers have had people change code on fairly stylistic issues without looking at big suggestions like ‘can we share this code with the code in class x’. In others the problem is the actual motivation for the change is not agreed. We need to try to get better at describing the big picture of a PR in some instances. Ask ourselves ‘what would it take to get this merged’
-
We should provide a way for people to pay for PR attention This has been proposed as 2 hour pre-purchased blocks of core team time to review a PR. It does not involve a commitment to merge and may take the form of discussion, a substitute fix and may result in a quote for more time if needed. But, it does mean that the CT member will spend 2 hours within a week on that issue or issues. (We should set up an easy mechanism for this if people do wish to take it up).
-
We should try to give timely feedback on what might be blocking a PR This is really a PR triage process. Often we are reluctant to comment on PRs because we know by doing so we may get drawn in or people. This ties into
-
We should meet weekly to triage the PR queue Often it’s really hard to do the above things in isolation. I propose we have a weekly meeting (or possibly 2 to accommodate timezones) to do PR queue triage.