On my first day at Trello, my peer mentor showed me two things. One of them ended up being the topic of my talk on Reactive Programming. The second thing he told me was, on our team, PR’s should tell a story.
Then we pair constructed a PR. To be clear, he was already in a local branch, and the code was already written and committed.
He showed me how he used git reset
and git rebase
to rewrite the commits. He explained that he was rebuilding the commits to make the PR easy to review. He constructed each commit to be atomic—to do a single coherent thing.
This was the PR style for our team for the whole time I worked there. The goal of a PR was to make it easy to review, and we expected reviewers to open each commit in a tab and review them one-by-one. We considered it acceptable to decline a PR if it was too big or hard to review (in practice, this happened very infrequently).
Other practices:
- PR’s were mostly less than five commits. If they were more, it was because each commit was tiny.
- Commits were tiny because we wanted them to be atomic—only one idea. For example, if I saw a spelling error in a comment, it would be its own commit. If I renamed a function, the rename refactor would be its own commit.
- Most PRs were merged within hours of being opened. Many even sooner.
- This is true even though we required 2 reviewers per PR.
- It only took 10-20 minutes to review a PR
- If PRs were big, we’d do a “Live review” over zoom to go back and forth quickly
- Single Jira tickets that required a lot of code were PR’d in parts.
- Almost every review comment was pointing out a blocking defect (or possible ones), not nitpicks.
- If the code changed the UI, we’d attach before and after screenshots.
These practices helped us, but the important part is our team mindset that PRs were constructed to be easy to review.