All code at Talis must go through peer review before it can be merged into
master. This can be done by
raising a pull request (henceforth “PR”) on the relevant GitHub repository and asking your colleagues for input.
30% vs 90%
Early feedback on an outline plan for your code is great. We follow the 30% feedback rule1 which allows developers to get early feedback on their approach before dotting the i’s and crossing the t’s. Simply label your pull request with a “30%” tag to get this kind of feedback.
Later, when you are closer to merge, and need a pedant, re-label your pull with a “90%” tag to get feedback on the details.
You’ll find that smaller changes (one liners or very small features) will skip the 30% stage and go right to requesting 90% feedback.
Authors may raise PRs prior to requesting a review - to track their own changes or organise their thoughts. If a PR has neither a “30%” nor “90%” tag, please do not comment unless the author asks you to.
How do I perform a Code Review?
Don’t always rely on the diff - depending on the size of the changes in the pull request, it might be a good idea to check out the branch locally. This gives you a number of benefits:
- You can open up your IDE and see the code as it was written. A variable may have been renamed - you can search in your IDE for the old values.
- You can spin up the app and take it for a test drive. The code might look ok, but the UI may look terrible and spam the console with errors, APIs might be hard to use, and it might run like a dog.
- You can run the local build (including tests) and see any potential breakages.
When can I hit merge?
There are no hard and fast rules about when a PR can be merged, except that you have to have had a round of feedback from a peer at 90% level, and responded to all raised issues either by committing further work or resolving any discussion in the PR itself.