Pull Requests from a Reviewer Perspective

Pull Requests from a Reviewer Perspective.md

Pull Requests from a Reviewer Perspective

Title

• The title should tersely describe what is being changed.

Description

• The description should fully describe what is being changed.
• More importantly, it should describe why this PR was created.

Picture is worth a Thousand Words

• If this is a large enough design change, add a diagram to the description.
• If this is a UI change, provide before and after screenshots.

Independent/Standalone

• There should be a single theme for the PR. A programmer typically fixes other stuff while in the code.
• Put the other stuff in a separate PR.
• The less noise a PR has, the easier it is for a Reviewer to merge it. So, please avoid fixing unrelated issues in one PR.

Tests

• Specs help convince the reviewer that the code works.

Examples

Small Bug Fix

• Single PR with 2 commits
  • Commit 1 should be a spec/test that reproduces the issue (red)
  • Commit 2 should be the actual fix that makes the above test pass (green)
• PR Description
  • if Bugzilla issue (e.g., 12345), should have standalone line with URL: https://bugzilla.redhat.com/show_bug.cgi?id=12345
  • if GitHub issue (e.g., 12345), should have standalone line with: Fixes #12345

Adding a Full-Stack Feature

Feature Overview

• it gets something new from a provider
• sticks it into the database
  • sql migration needed to add a couple of columns
  • sql data migration needed to adjust for old vs new way of storing information
• displays it in the UI

Original submission as Single PR

• with new code sprinkled in db/migrate, app/models, app/controllers, app/views
• with specs for data migration and model only
• with some whitespace changes (because the developer saw the ugliness and fixed them locally)
• with some refactoring (because developer says "while I am in this area of the code, ...")
• Reviewer is having a hard time grokking all these inter-related changes
• As reviewer comments on each of the code areas, author must fix each area
• So, even if review on certain area (e.g., sql migration) is complete, PR does not progress
• The whole PR is stalled or moves very slowly in this process

Better approach as Multiple PRs

• 1 PR for whitespace changes
• 1 PR for refactoring
• 1 PR for sql migration to add a couple of columns
• 1 PR for sql data migration needed to adjust for old vs new way of storing information
• 1 PR to get data from provider with specs
• 1 PR to add data to database with specs
• 1 PR to display information in UI with specs

Benefit of Multiple PR approach

• optionally create a GitHub issue providing an overview of how all the related PRs would work together
• the smaller each PR, the easier it is to review
• the smaller each PR, the fewer comments on each PR
• the smaller each PR, the less discussion needs to be had
• the smaller each PR, the easier it is to see relevant specs/tests
• the smaller each PR, the faster it will get merged

Changing the interface of some piece of code

Refactoring