Introduction
This document describes how we use Pull Requests at Township, and what comprises a well-formed PR.
Pull Requests at Township
The purpose of a PR is to provide a summary/preview of the work that has been done to those doing code review for a feature, bug fix, code change, etc.
A PR should consist of as many of the following as needed:
- Title
- Start with a feat, fix or chore tag (conventional commit style)
- Match the Linear ticket title somewhat, if possible
- Tag drafts with WIP so its apparent the PR is not yet ready to be reviewed.
- Description
- Summary of the reason for the change/new feature/bug
- Summary of the approach taken, reasoning behind engineering decisions
- Summary of what your change/feat/fix accomplishes
- Loom video/Screen Recording
- Demo of the changes, walkthrough, setup, etc.
- Try to keep this under a couple mins or less.
- Use a screen recording if no verbal explanation is needed and it is something quick.
- Screenshots
- Highlight before/after of a UI addition/fix/update etc.
- Add label to screenshot
- List of files added
- Ex: adding a bunch of icons may look like a lot of code has been added when in reality it’s just a lot of new files. It could be helpful to list those in the PR description so the code reviewer knows those may not need to be looked at in detail
- Links
- to Figma if its a UI ticket
- to Linear - especially if the project is not synced with Linear
- to newly added dependencies
- to reference articles used
Pull Request Process at Township
Submitting PRs
When you push your first commit from a branch, Github will display a banner with a button to open a new pull request. You then need to fill in the PR description and submit it as a Draft PR or Ready for Review PR.
Draft PRs
- start a draft when you’ve pushed up some code but aren’t ready for final review
- start writing the full PR description, update as progress is made/commits are added
- maybe include to-do items
- include blockers/what you’re having trouble with & need help on
- link to the PR in the project slack channel to draw attention to it if help is needed
Ready for Review PRs
- Change Linear ticket status to “Ready for Review”
- Cleanup commits (Rebase/squash)
- Make sure your branch is pointing to the right parent branch, and is based on its most recent commit
- Convert an existing draft to Ready for Review
- Make sure CI has passed
- Tag reviewers
- Tag yourself as assignee
- Wait for feedback & comments
- Address requested changes
Addressing Requested Changes
When in doubt, re-request feedback after a code reviewer has requested changes on your PRs.
Nits/Cleanup
These are nice-to-haves, but may not be required. The code reviewer will usually specify this (ex: “maybe make these small changes then it’s good to merge”).
Reviewer likely doesn’t need to re-review after these changes.
- Fixing indentation
- Better function naming
- Adding/cleaning up comments
- Removing left-in debug statements
Refactoring
These changes should get reviewed again after fixing. What you’ve done works, but could maybe be written more concisely.
Re-request review after addressing feedback, unless the reviewer specifies otherwise.
- Cleaning up repeated code
- Maybe changing an approach/function design
Required Changes
These changes must be made before the PR can be merged, and will possibly get multiple rounds of CR.
Always re-request review after addressing feedback.
- Found bugs
- Missing functionality
- New/updated requirements for the ticket that now require additional code change
Merging PRs
Requirements for merging:
- 2 approved reviews - typically with an explicit “looks good to merge” (LGTM) note
- Possibly only 1 required - with approval from ? Scott? ex: very few people on a project, maybe can’t realistically get 2 reviewers in time
- No merge conflicts!
Performing the merge:
- Hit that Squash & Merge button
- Update ticket status in Linear to In Review (if not automatically done by Github)
Additional Reading
Library
This is a new feature PR. There were a lot of files added/changed so aside from the description, loom video, figma links and screenshots, files added/files changed section was added to make it easier for the review. Some of the files were simply icon files so they didn't need to be reviewed as thoroughly. Sometimes you have to touch a lot of making reviewing your PR time consuming. This PR also had a conversation between the reviewer and the assignee regarding suggested changes to the PR.
A bug fix PR that describes the problem in detail, explains the desired behavior, and summarizes how these changes apply a fix.
