Guide for Reviewers - MetOffice/stylist GitHub Wiki
Reviewing a Change Request
General information regarding tools and expectations of the code is given in the developer's guide in the main documentation. This page holds notes to aid in reviewing change requests. It is expected that this page will evolve over time as experience is gained.
Branch and Merge Process
We follow the same basic process as most projects on GitHub. The developer takes a clone (or fork) of this canonical repository into their own developer collection. They will then clone (or fork) this to their development platform where they will prepare changes.
Once they are happy with their local changes they will push these to their GitHub repository. From here GitHub will offer them the opportunity to "raise a pull request." That request will then appear here.
Review Process
Some of the review process is automated. The source will be passed under the noses of Flake8, MyPy and pytest. If any of these fail it will not be possible to merge the changes on to trunk.
The job, then, of the reviewer is to be alert for those things which cannot be automatically tested. Poor design, code smells, clumsy implementation and other important aspects.
It is normal for there to be a few rounds of back and forward between developer and reviewer. Once the reviewer is content that the change is good enough to go on trunk they approve it.
The Final Merger
By default a change needs only a single reviewer to pass it. For big or complicated changes it may be decided that multiple eyes need to pass over it.
Once all approvals are in it is the responsibility of one of the reviewers to merge the change. This is done for us by GitHub using a "squash" merge. We don't need to see all the intermediate stages which lead to a change, just the final change.