Tree Hygiene

Introduction

This page covers how to land a PR and other aspects of writing code for MQReawakened other than the actual writing of the code. For guidance on documenting and formatting your code, see the style guide document.

Overview

The general process for submitting code to MQReawakened is as follows:

1. Fork the repository on GitHub (see the contributing guide for advice on doing this and in general setting up your development environment).

2. Create a branch off of the master repository on your GitHub fork, and implement your change. Make sure it is tested. You must follow the guidelines described in the style guide for the MQReawakened repo.

3. Submit this branch as a PR to the head repository.

4. Get your code reviewed. You should probably reach out to the relevant expert(s) for the areas you touched and ask them to review your PR directly. GitHub sometimes recommends specific reviewers; if you're not sure who to ask, that's probably a good place to start.

5. Once everything is green and you have an LGTM, land your patch.

6. Watch MQReawakened and see if it runs into any problems. If anything goes wrong, revert your patch and study the problem. You should aim to be the one to revert your patch. You will be racing everyone else on the team who will also be trying to revert your patch.

Getting a code review

Every PR must be code-reviewed before check-in, including things like rolling a dependency. Getting a review means that a regular MQReawakened contributor (someone with commit access) has written a comment saying "LGTM" on your PR, and you have addressed all their feedback. ("LGTM" means "Looks Good To Me".)

If nobody reviews your PR within two days, you can comment to ask for someone to review it, and it should be bumped.

Code review serves many critical purposes. There's the obvious purpose: catching errors. Even the most experienced engineers frequently make errors that are caught by code review. But there are also many other benefits of code reviews:

• It spreads knowledge among the team. Since every line of code will have been read by two people, it's more likely that once you move on, someone else will understand the code.

• It keeps you honest. Knowing that someone will be reading your code, you are less tempted to cut corners and more motivated to write code you are proud of.

• It exposes you to different modes of thinking. Your code reviewer has probably not thought about the problem in the same way you have, and so may have a fresh perspective and may find you a better way to solve the problem.

We recommend you consider these suggestions for addressing code review comments on your PR.

If you're working on a big patch, don't hesitate to get reviews early, before you're ready to check code in. Also, don't hesitate to ask for multiple people to review your code, and don't hesitate to provide unsolicited comments on other people's PRs. The more reviews the better.

A reviewer may in some circumstances consider the code satisfactory without having fully reviewed or understood it. If a reviewer has not fully reviewed the code, they admit to this by saying "RSLGTM" rather than just "LGTM". If you feel your code needs a real review, please find someone to actually review it. ("RSLGTM" means "Rubber Stamp Looks Good To Me".)

How to review code

Reviewers should carefully read the code and make sure they understand it. A reviewer should check the code for both high-level concerns, such as whether the approach is reasonable and whether the code's structure makes sense, as well as lower-level issues like how readable the code is and adherence to the style guide. Use these best practices when reviewing code and providing comments.

As a reviewer, you are the last line of defense.

1. Take a step back. What problem is the PR trying to solve? Is it a real problem?

2. What other solutions could we consider? What could we do to make this even better?

3. Is it the best implementation? Again, see our style guide, in particular its section on good coding patterns. Are there hacks? Are we taking on more technical debt? Think of ways in which the code could break.

4. Is it testable? Is it tested? All code must be tested.

5. Look for mistakes in indenting the code and other trivial formatting problems.

6. Is the documentation thorough and useful?

7. Check for good grammar in API docs and comments. Check that identifier are named according to our conventions.

Once you are satisfied with the contribution, and only once you are satisfied, write a comment that includes the phrase LGTM (or use the GitHub "Approval" mechanism). If you feel like you are being worn down, hand the review to someone else.

In general, reviewers should favor approving a PR once it is in a state where it definitely improves the overall code health of the system being worked on, even if the PR isn't perfect.

Reviewers should always feel free to leave comments expressing that something could be better, but if it's not very important, prefix it with something like "Shouldn't block this PR but: " to let the author know that it's just a point of polish that they could choose to ignore in the current PR (these should be documented in TODO comments with a tracking issue).

If you are 100% sure the patch is good and has no issues but you haven't really fully reviewed or understood it, you can give it a rubber-stamp review by marking it RSLGTM. If you mark a patch as RSLGTM, you are still sharing the responsibility for the patch with its author. Reviewing a patch as RSLGTM should be a rare event.

If you are not a regular MQReawakened contributor (someone with commit access), we very much welcome your reviews on code contributions in the form of comments on the code, but please refrain from approving or LGTM'ing changes, as it confuses PR authors, who may think your approval is authoritative and merge the PR prematurely.

Landing a patch

Once you have submitted your patch and received your LGTM, if you do not have commit access to the repository yet, then wait for one of the project maintainers to submit it for you.

If you do have access, you can just click the green "Merge pull request" button on the GitHub UI of your pull request. Please squash commits (GitHub does this for you by default normally).

Squashing commits

When you squash commits, by default, GitHub will concatenate all your commit messages to form a unified commit message. This often yields an overly verbose commit message with many unhelpful entries (e.g. "fix typo"). Please double-check (and hand-edit if necessary) your commit message before merging such that the message contains a helpful description of the overall change.