Skip to content

Home

Jump to bottom
Neil Kalman edited this page Jan 6, 2016 · 1 revision

Some rules about Pull Requests

use the following template in your PRs:

# Change Summary
< single change as paragraph >

< multiple changes as checkboxes. if a bug was introduced or a change will be implemented as a follow up, include an unchecked checkbox
- [x] change short
- [x] change short
- [ ] change short
>

< if needed! >

# More info

## Change title

< description >

This will make a PR clearer on what you decided to leave out and what you actually did.

Try to describe what you did, not what you changed. Technical descriptions can be added if necessary, but try to explain things that can't be seen by just looking at the code itself.

Example:

  • removed file
  • changed variable name

are unnecessary descriptions. looking at the PR's diff will tell the reviewer that part of the change. Try to describe things that the PR reader would understand what task you did and why

  • Created a new component
  • Added double click functionality to old component

If you introduced a bug, or something that is planned to be added in a follow up PR, note it in the list with an un-checked checkbox.

Example:

  • Created a new component
  • Styling for new component is half baked. Will fix in a follow up PR

If something needs further explanation (more than one line), add details in the More info section.

Example:

  • Styling for new component got messed up (see details below)

More info

Styling new component

I didn't fix this because ...

If you want to write any clarifications for a specific line of code (and you don't want it to be a code comment), you can also add comments inline (so the Code Reviewer will see it when he starts looking at the code)

Tips for better PRs

1. Make it small

A small, focused Pull Request gives you the best chance of having it accepted.

The first thing I do when I get a notification about a Pull Request is that look it over to get an idea about its size. It takes time to properly review a Pull Request, and in my experience, the time it takes is exponential to the size; the relationship certainly isn't linear.

How small is small enough? Obviously, it depends on what the Pull Request is about, but a Pull Request that touches less than a dozen files isn't too bad.

2. Do only one thing**

Try making each PR have one big change and no more then a couple of bug fixes.

Imagine, as a counter-example, that you submit a Pull Request that addresses three independent, separate concerns (let's call them A, B, and C). The reviewer may immediately agree with you that A and C are valid concerns, and that your solution is correct. However, the reviewer has issues with your B concern. Perhaps he or she thinks it's not a concern at all, or she disagrees with the way you've addressed it.

This becomes the start of a lengthy discussion about concern B, and how it's being addressed. This discussion can go on for days (particularly if you're in different time zones), while you attempt to come to agreement; perhaps you'll need to make changes to your Pull Request to address the reviewer's concerns. This all takes time.

It may, in fact, take so much time that other commits have been merged into master in the meantime, and your Pull Request has fallen so much behind that it no longer can be automatically merged. Welcome to Merge Hell.

All that time, your perfectly acceptable solutions to the A and C concerns are sitting idly in your Pull Request, adding absolutely no value to the overall code base.

Instead, submit three independent Pull Requests that address respectively A, B, and C. If you do that, the reviewer who agrees with A and C will immediately accept two of those three Pull Requests. In this way, your non-controversial contributions can immediately add value to the code base.

The more concerns you address in a single Pull Request, the bigger the risk that at least one of them will block acceptance of your contribution. Do only one thing per Pull Request. It also helps you make each Pull Request smaller.

3. Watch your line width

ℹ️ With Kibibit, this will also make a test fail ℹ️

We prefer to do code reviews using Reviewable.io. You can also review a PR using Github itself. GitHub, ungit, and Reviewable provide browser-based diff views for reviewing. A reviewer can even configure the diff view to be side-by-side; it makes it much easier to understand what changes are included in the contribution, but it also means that the code must be readable on half a screen.

If you have wide lines, you force the reviewer to scroll horizontally.

There are many reasons to keep line width below 80 characters; making your code easy to review just adds another reason to that list.

4. Write well

Write good code, but also write good prose. This is partly subjective, but there are rules for both code and prose. Code has correctness rules: if you break them, it doesn't compile (or, for interpreted languages, it fails at run-time).

The same goes for the prose you may add: Code comments. Commit messages. Pull Request messages.

Please use correct spelling, grammar, and punctuation. If you don't, your prose is harder to understand, and your reviewer is a human being.

Clone this wiki locally