Contributing to new repositories that your team does not own can feel daunting, and raising your first merge request can feel like a genuine hurdle to be passed.
In this post, I will give some suggestions on what can make the review process go more smoothly.
Document why you are proposing the changes
In the description of the merge request, ensure you include the reasons for the change. This is the most critical piece of information that can help the person reviewing understand the context in which the proposed change is made. Including any screenshots of the functionality or any additional documents or ticket links are a great addition.
Try to avoid explaining what the change is - that should be self-evident from the code; giving the reviewer the context of why the change is happening should be enough for them to decide if your approach is appropriate or suggest an alternative.
Clean commit history
What does a clean git history mean? For me, this would entail two main issues:
- Atomic commits - if you cherry-pick any of the commits, the commit has to make sense on its own, and tests have to pass on each commit.
- Clear commit messages - prefer longer commit messages (don’t be afraid to have multi-line messages!) and explain why changes are made.
Having a messy git history will make it harder for the reviewer to see the progression of the changes over time. For most PRs, all of the changes can be held within one commit, which makes keeping the history clean very easy.
Separate functionality and test changes
Suppose you land your change request in one of the below three categories:
- Refactoring application code
- Refactoring or adding tests
- Adding of application code
In those cases, it makes reviewing the PR a lot easier due to the implications on the overall functionality of the app: you are maintaining existing behaviour.
There is a fourth category - the bug fix, which poses the most risk as when deploying the code there is a risk of introducing regressions, as you are changing the existing behaviour of the application.
Refactoring of application code
Refactoring of application code should mean that there are no changes to the tests. If you notice missing tests while refactoring, open a change request where you add the tests and only afterwards change the functionality.
When one refactors code, the functionality provided should match the previous specification, and as such, no new tests should be required.
Refactoring or addition of tests
Refactoring/Adding tests implies that no application code has been changed. These are the most straightforward change requests to review - everyone appreciates additional test coverage!
Addition of application code
This is by far the most common change request. This requires more attention from the reviewer to ensure that the new tests correctly cover the new functionality and that there is no interaction between existing code and new code that is not covered by existing tests - or if the interactions exist they should be covered by new tests.
The bug fix
This is a change request where the application code is updated to fix a current issue. Usually, this will imply updating existing tests; however, for a fair few bug fixes, there will be no existing tests. If there are no tests, it would be preferable to add tests first that showcase the wrong behaviour and then proceed to fix them.
This PR is the one that requires the most context and where the burden of proving why the change is required is very high. Explaining why, and showing how by adding tests, the current code is broken is more important than in any of the previous cases. This change request will change existing behaviour, so we need to ensure the bug fix correctly resolves the issue.
Conclusion
The above are general recommendations that can apply to any PRs. If you are to take away only one thing from this post, you should always document why changes are being made - the code should be self-sufficient for understanding what changes are made.
Author: Alexandru Stoica