Merge Requests
Merge Requests, are self-contained changes that have been submitted to version control or which is undergoing code review. Other organizations often call this a "change", "patch", "pull request", "changelist" or "MR".
Creating MRs
When creating MRs, we should try to follow a set of standard rules:
MRs should be self-contained work - This usually means adding a feature, changing an existing feature, or fixing a bug. Any changes in the MR that are not self-contained should be removed and put into a separate MR. This results in smaller MRs, which have multiple benefits:
Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small MRs than to set aside a 30 minute block to review one large MR.
Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the MR and see if a bug has been introduced.
Less wasted work if they are rejected. If you write a huge MR and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.
Easier to merge. Working on a large MR takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current MR in review.
Simpler to roll back. A large MR will more likely touch files that get updated between the initial MR submission and a rollback MR, complicating the rollback (the intermediate MRs will probably need to be rolled back too).
MRs should be rebased onto the main/master branch - At Nexus Mods, we utilise a semi-linear merge git flow. This essentially just means that the main/master branch should reflect what is in production and work is constantly being rebased ontop of it. MRs should always be rebased onto the latest master commit before being reviewed. See Git Workflow for more information.
Flag the MR as Not Ready/WIP if it's not ready to be reviewed - sometimes branches for MRs might not be fully complete, or developers might be pushing changes to ensure that their work is not lost. By default, all MRs should be considered WIP, until they have been submitted. This can be done by ensuring the MR's title starts with
WIP:orDraft:, this will also block the MR from being merged.
Submitting MRs
All code should work
The code has to work (at least to the best of your knowledge). The code should be tested locally, in terms of running the code locally, as well as adding unit/integration tests to cover any new code.
Ideally, all tests and CI processes should have succeeded before requesting a review.
Having people review your code is one thing, but you should not expect them to also test the code for you.
Choose a reviewer
Once you are happy with the MR you should get the attention of another developer to review. There are multiple ways of achieving this and developers are expected to use common sense and use their own judgement based on the urgency and size of the MR.
For MRs that need urgent review E.g. in the case of an Emergency, you should actively encourage a specific developer to review. This could be as simple as messaging them (A simple slack message would be fine for this E.g.
@john_doe can you please review https://<Link to MR>), or simple asking another developer in person to review.Posting a message in the appropriate team's Slack channel would be sufficient if the issue is less urgent and/or requires multiple reviewers. E.g. if multiple teams need to review something.
For low priority MRs, simply adding the
Stage::Reviewlabel to the MR will indicate that this MR is ready to review, and should be picked up by available reviewers when they have time.
Ultimately, it is the responsibility of the author for the merge request to be reviewed. If it stays in the ready for review state too long it is recommended to request a review from a specific reviewer. When choosing a specific reviewer, try to remember the following:
Try to select " domain experts " for the code you want them to review. These are developers who usually have experience in the specific section/project you are working on.
Avoid repeatedly choosing the same person for reviewing your code. This just encourages "siloing" of information, and ideally, we want to be getting as many people looking over code as possible. However, sometimes this isn't possible as the reviewer should be someone who is familiar with the section / project you are working on. These "domain experts" might sometimes be a single person, so it's unavoidable to always choose them. In this case, it might be worth asking the "domain expert" as well as an additional developer to review to encourage information sharing.
Sometimes specific reviewers are not available (sickness, holidays or simply just too busy to review an MR) and that's fine! In this case, and in any case of doubt who should review something, asking multiple people to look over and review code is perfectly reasonable and is encouraged.
In-Person Reviews (and Pair Programming)
If you pair-programmed a piece of code with somebody who was qualified to do a good code review on it, then that code is considered reviewed.
You can also do in-person code reviews where the reviewer asks questions and the developer of the change speaks only when spoken to.
Reviewing MRs
All MRs should be reviewed where possible. See Code Reviews for more information.