Merge Requests

Here follows some advice for dealing with merge requests, both as the opener and as a reviewer.

Opening a merge request

For technical information about opening a merge request, see here.

You can make the review process faster and simpler by taking the following advice:

  • Provide a brief description for your MR to make the job of reviewers easier. You might want to include some of the following bits of information:

    • What the MR tries to achieve
    • Why you chose the implementation approach you did
    • Screenshots or a video of the change in action
    • Links/file references to the parts of the MR that need particular attention (perhaps due to complexity)

  • Offer to review someone else's MR in exchange for them reviewing yours: you don't have to be a core developer to provide a review, and it takes pressure off core developers who often find themselves busy. A core developer will still need to provide the final 'rubber stamp', but good peer review can turn this into a quick formality rather than a longer wait.

  • If you feel that a specific contributor is well-suited to reviewing your work, and they've shown evidence of recently being active, you can send them a message asking for them to review. Sometimes things get missed and just need a little prompting!

  • Keep MRs small and self-contained. If you change too much in a single MR it can become difficult to review and potential reviewers might be more reluctant to prioritise it. A common approach is to split large features into smaller MRs, with a compile-time flag that disables the sub-features until everything is ready to enable them.

  • Remain open-minded about feedback: nobody wants to stand in the way of progress, but controversial changes might cause an MR to get stuck in review for longer. It might be better to put any potentially controversial changes in an MR of their own to avoid them blocking the rest of your changes.

Reviewing a merge request

You - yes, you - can review a merge request!

Contrary to popular opinion, you do not need to be a core developer to review / approve a merge request! Any contributor can perform a review, and community peer review is strongly encouraged because it takes pressure off core developers, who may be busy with other things.

You also do not need to have a deep knowledge of the codebase to successfully review an MR. Often, you don't even need to know how to write code at all! The following kinds of review are just as useful as code review:

  • Play-testing (checkout the branch, give it a whirl to see if everything feels right)

  • Balance review (are the changes balanced and well-matched with existing gameplay mechanics?)

  • Aesthetic review (are the changes in-keeping with the existing aesthetics of the game?)

  • Sniff test (does the code look like it's doing the right thing?)

  • Partial review (review the bits you're familiar with and ask for help for parts you feel less comfortable reviewing)

Code smells

One of the benefits of using Rust is that the scope of possible problems a change can introduce is much smaller. Even if you only have a passing familiarity of the language, you should still feel empowered to review code.

As a rough guide, here are some 'code smells' that you might want to watch out for:

  • Use of unsafe. This is generally a big no-no and its use should be carefully reviewed by a core developer.

  • Use of unwrap. Each use of unwrap should be justified with a comment explaining why it can't cause a panic.

  • Cloning/creating items. Avoiding item duplication exploits is very important.

Review checklist

If you're new to reviewing, here is a short checklist of things you might want to do as you review a set of changes:

  1. Make sure the change is desirable and in-keeping with existing game mechanics.

  2. Play-testing. Check out the branch, kick the tires. Try to break it. Take note of any bugs, strange behaviour, or things that feel unintuitive.

  3. Skim the code. Are any of the changes non-trivial enough to need more careful checking? Do they look like they fit with the existing code? Are things implemented in the right places?

  4. Ask a domain expert - or the MR author themselves - for help if there's anything that you feel less confident about approving.

  5. Do any of the changes mean that documentation needs updating to match? Has this been done?

  6. Are the changes properly localised? All user-facing text should be generated by the localisation system. Only an English translation needs to be provided for merge, however.

  7. Has the changelog (CHANGELOG.md) been updated if the change is use-facing? Note that internal refactors and trivial changes do not need to be added to the changelog.