---
title: Git - Pull Requests
description: Best Practices for Git Pull Requests
tags: git, best practices, code reviews, pull requests
lang: en
---


> * [Thomas Burleson](mailto:[e-mail]), Solutions Architect
> * Copyright 2022 - All rights reserved
>

<br>

# Knowledge Pyramid

The illustration below highlights the layered training process used to scale teams to Enterprise Git Best Practices:

<br>


<br>
<br>

# Git Guidelines: Pull Requests

Developers are often confused by the concept of Git Pull Requests. While the concept is simple, team often struggle with Pull Requests (PRs). There are, however, **best practices** that will ensure your PR process is pain-free and clean.

* Use recommended [GitFlow Feature Branches](https://hackmd.io/mqAqzpKyTqmQuofFZkkHxQ?view#1-Use-GitFlow-Strategy).
* Use Pull-Request Best Practices (this document)
* Use Code Review Best Practices

<br>

<br>

## Why Use Pull Requests?


So when do developers need to use PRs?

Pull requests are critical when the author (developer making the changes) wants to:

1. <u>Merge code</u> from the current feature branch to another branch
3. <u>Share ideas & approach</u> with **specific** developers
4. Request <u>code reviews</u> from **specific** developers
5. <u>Demonstrate</u> quality and standards
6. <u>Validate</u> with cloud CI processes
7. Enable code owners to <u>sign-off</u> on changes or enhancements

<br>

Pull Requests can even be used across different repositories.

For example, developers could easily create a PR to merge code from `repo1/feature/fix-ui-login` ⮕ `repo2/develop`.

:::success
What is the _secret weapon_ to great Pull Requests? **Rebasing**
:::

<br>

## Best-Practice Rules for PRs

<br/>

### 1. Create a Pull Request

#### Video: Creating a Git PR

When creating a Pull Request here are the guidelines:


<br>

* Code changes/work **must be in a Feature branch**
* Avoid large quantities of changes in a single PR
* Squash the commits to 1-3 commits
* Commits must conform to [commit conventions](https://hackmd.io/@thomasburleson/BJrUV8ssr#-Commit-Message-Conventions)
* If UI changes, provide Before/After images in message body
* must contain `Refs #<xxx>` or `Fixes #<xxx>` footer notes
* Rebase is required (using `git pull --rebase origin <destination-branch>`)
* squash before rebase to reduce conflict issues
* Local repo passing **prettier**, **build**, **lint**, and **tests** checks.
* use Prettier to minimize diffs and styling issues
* if the PR includes non-UI, business logic changes
* should have 1..n unit tests
* may need 1..n integration tests
* if the PR includes UX changes,
* should have associated e2e test (if applicable)
* should have BEFORE / AFTER snaphshots in the commit message
* The PR description should clearly articulate the PR intention and context:
* describe what has changed or be added/redacted
* describe how to use the feature (animated GIF is very useful)
* for UI changes, include BEFORE/AFTER snapshots of relevant UI changes
* include breaking API changes
* any additional notes that will help the reviewer
* The PR must have assigned at least (1) **Reviewer** and (1) **Code Owner**
* if changes in shared libraries, extra care is required
* must have associated code owner approval
* do NOT include "all" team members to review...
* choose 1-2 reviewers
* do not always use the same reviewer

<br>


### 2. **WIP** Pull Requests

For Pull Requests that are not yet ready for a formal review or comments, a Work-in-Progress (WIP) label should be used. A WIP PR is purposed to share ideas online or demonstrate interim progress with a non-trivial change.

WIP Pull Requests:

* should only have WIP if the PR is used to preview or share ideas
* should be considered <u>NOT READY</u> for review
* developers should NOT comment on WIP unless solicited by the authorauthor
* these PRs are consider incomplete may fail any or all tests

<br>


### 3. Open Pull Requests

#### Video: Using WIP PRs


#### Guidelines

* Use labels (needs-review, ready-for-merge, etc)
* Can only be merged by SEL (or designatee)
* Must pass CI tests/lints before ready for merge
* should not have `it.only()`, `fdescribe()`, `fit()`, `xdescribe()`, or `xit()` tests
* should limit # of disabled tests
* Should have [**recent rebasing**](https://hackmd.io/mqAqzpKyTqmQuofFZkkHxQ?view#3-Rebase-Often) from `<destination>` branch
* Should have at least one code review
* must have code review approval before assigning LGTM labels
* should expect the code review to be performed in < 24 hours
* For urgent PRs, the reviews and authors should be notified regarding the urgency.
* Should follow documented Code-Review practices

<br>


### 4. Approving Open PRs

* see the section below for Code-Review best practices
* should be marked with LGTM by Reviewer and Code owner
* should pass all tests
* if the PR contains multiple commits
* squash commits into 1 commit or logic set of commits
* for clarity, multiple commits are squashed into logical groups

<br>


### 5. Merging Approved PR

#### Video: Merging PRs


#### Guidelines

* should NEVER use the default 3-way merge. Always use the [**fast-forward merge**](https://hackmd.io/mqAqzpKyTqmQuofFZkkHxQ?view#(4)-Use-Fast-Forward-Merge)
* always maintain a linear commit history
* archive/delete associated feature/fix branch after merge
* merge (after rebase) with ff-only; keep commit timeline FLAT

<br>


### 6. PR Cleanup

* delete the `origin/feature/<xxxx>` branch
* Note: This can be done automatically via repo settings post-merge

# To delete the remote branch `thomas/conversation`, use
git push :thomas/conversation