Skip to content

Instantly share code, notes, and snippets.

@daniel-barlow
Last active June 18, 2020 08:15
Show Gist options
  • Star 2 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save daniel-barlow/0bf1be1988a8681312e64738676c37e4 to your computer and use it in GitHub Desktop.
Save daniel-barlow/0bf1be1988a8681312e64738676c37e4 to your computer and use it in GitHub Desktop.
What to put in a Pull Request
As a developer
I want to get better at making my PRs easier to review
So that they get reviewed sooner and I can merge faster => shorter cycle times

As a code reviewer
I want PRs that are easier to review
So that it's less work

I wish to claim that spending extra time as developers on writing PRs is repaid in time saved by the people reading them - if only because they are read more often than written. Here are some of the things I now try and remember to put in on each one. Obviously not all points are relevant to all changes in all projects.

I would like to hear your opinions: as a reviewer, are there other things you would really like to see in PR messages that you don't, or are there things I'm proposing to write here that you really don't think are valuable to read? As a contributer, does this seem too little/about right/too onerous?

What it does

Summarize the changes that this PR will make to the application, from a developer's perspective. e.g. New/changed/deleted classes, methods, API calls, messages. Where you know it will impact on other specific teams/projects (e.g. Analysis or Systems), mention them specifically.

Why it is important

Make a case for merging this work. Give enough context that somebody outside your track/team (e.g. in Tech Leadership or App Support) would be able to understand why it is valuable

Implementation notes

  • How to read the PR when reviewing (e.g. "start at the top", "start with models/foo.rb", "ignore app/presenters changes, this is all rubocop noise", or "I would like you to review each commit individually, as I put real effort into my commit messages")

  • Decisions you made about the tech approach: if other approaches were considered, why you did it this way instead

  • Decisions that you are unsure about and/or particularly want a second (or third) pair of eyes on.

  • _What cross-team collaboration was needed and has already happened: noting this ("I talked to @madeupname in Systems and they are happy") will head off questions asking if you've asked them

  • anything noteworthy about how or when to merge and deploy this work - e.g. other PRs that should be deployed before/after it, does it require service downtime.

Future directions

Note anything this PR does not do which a reviewer might otherwise wonder if it should. Make sure to say whether it's work which is in your team's (or another team's) backlog already, or work which you think there's a case for doing but that has not been agreed.

  • code quality issues it uncovers or refactors it enables

  • future new Biz functionality which is unlocked/enabled by this change

_There is no need here to recapitulate your entire product roadmap - this is more for questions at the scale of "are you going to fix those style errors/calls to deprecated methods?" than "are you going to redesign the User model?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment