Document how to get something reviewed
Created by: eileenmcnaughton
Removed this page https://docs.civicrm.org/dev/en/latest/core/release-process/ & add a page on how to get something reviewed @mattwire @colemanw @totten first cut -
How to get your code contribution reviewed
So you've analysed a bug, written a fix, and created a pull request - now what?
Getting a code change merged into core requires someone to review it and someone to merge it. Depending on the complexity of the change those could be the same person or multiple people might be involved.
Who does the merging & review
The reviews are done by the core team and community volunteers. In general anyone can help with review although depending on the PR the merger may still have to put in time to be satisfied the PR is safe and appropriate.
In many cases the time that goes into reviewing /merging is considerable and it is not uncommon for more time to go into the review process (both yours and the mergers) than the time it took you to create the PR. Merging a PR also puts the reviewer and / or merger 'on the hook' they will feel obliged to fix any bad side effects if they occur (although they would hope that you would do so first) and also they may have to justify their decisions if the change does result in a regression.
For the reasons above, and, also because in many cases reviewing and merging PRs requires skill and experience, reviewer time is a bottleneck in the system. Overall more PRs are generated than there is capacity to review :-(
So what can you do to ensure your PR gets attention
If you want to 'outcompete' the other PRs for reviewer time there are some things you can do
- respect the reviewer time. Respond to feedback courteously and promptly. Make yourself someone the reviewer sees as a good use of their time.
- write good issue descriptions and steps to replicate, use screenshots.
- include unit tests. Reviewers very actively target the PRs with the 'has-test' flag.
- conversely if the code requires unit tests to be merged and you don't have time to write them then close the PR until you do and track it in gitlab. Rightly or wrongly reviewers are likely to think that if you have lots of PRs open with the 'needs-test' flag you are expecting someone else to write one for you.
- keep your open PRs in a reviewable state - gitlab is the place to track work that is not yet reviewable and by keeping your open PRs limited to ones that are reviewable you will get seen as a good time investment. (You can close a PR without losing your work). It's fine to open a work-in-progress PR for a discussion but it should only stay open for a week or so or while active discussion is taking place on it - a closed PR is as good as an open PR for discussion but doesn't create a burden on the triage process
- review other people's PRs, either as a direct trade or as part of the overall effort
- connect with people with interests in the same code and bugs and ask them to review your PRs
- sometimes fixing someone else's bug while you are at it is a good way to lure them into testing your PR..
- make your PR address the product maintenance code objectives
- clean up the code prior to making the change. Generally the clearer the actual change being made is the more comfortable the reviewer will feel. Preliminary clean up PRs can pave the way for the actual change to be small and to get the attention you want it to.
- keep your PRs small and separate. Often a reviewer has a spare hour for code review. If you have a number of small clean up PRs open they might pick a few off. If you you have one larger PR with several changes in it they will likely skip it as too much to do in the time they have. Reviewers are rightly scared of missing gotchas in large code changes
- always do re-formatting or big code moves as a separate function
- extract functions often. Extraction PRs are a great way to make code more readable - they also are relatively easy to get merged as the reviewer just has to re-do the extraction themself & see if they get the same result.