Creating and reviewing pull requests in [#git]

Creating and reviewing pull requests (PRs) are important parts of collaborating on projects through LinkORB’s Git repositories. While the team is always looking to automate adherence to styling and quality standards using linters and other quality assurance tooling, human focus and review will always be required.

The purpose of this guide is to provide an outline of:

• What to do when creating a pull request
• What to look for when reviewing a pull request

Specifically this guide will address:

Small, distinct pull requests are better

Large pull requests are more cumbersome to review and have a greater chance of introducing unintended side effects.

Similar to the principle of separation of concerns, a pull request should address a single issue.

For example, adding a feature and upgrading unrelated project dependencies to in the same PR is not good practice. Upgrading dependencies might break something else, result in unpredictable behaviour, and make testing more complex.

Similarly, adding a feature and fixing an unrelated bug in the same PR is not good practice.

However, adding new dependencies and a feature in one PR if the dependencies are required for the feature is fine.

Pull request titles and descriptions are concise

Pull request titles and descriptions should assume the reviewer has little-to-no existing context.

PR title best practices

A pull request title should be:

• Written in easy to understand language.
• Summarize the PR’s contents.
• Begin with a conventional commit type. See commit types for more information.
• Use the dominant conventional commit type that communicates the overall intent of a PR if the PR contains multiple commit types.
• Terminate with # and the Team HQ card number associated with the PR. For example #1234.

PR description best practices

When describing a pull request, please avoid shorthand, acronyms, and abbreviations. Rather, use complete sentences and a linear explanation of what the pull request includes.

All repositories in the LinkORB organization provide a template for pull request descriptions, requiring information such as:

• An explanation of the proposed changes
• The types of changes/commits included
• A checklist acknowledging self-review steps taken

Only include relevant and readable files in PRs

Changes to files such as lockfiles and other peripheral files are sometimes included in PRs when they are not relevant or required for the PR. If you are creating a pull request, ensure the PR only contains relevant and necessary files.

Any file you intentionally include in a PR should create a readable and intelligible diff; meaning a reviewer can clearly see the changes made. If the diff is not intelligible, it is not reviewable and therefore cannot be merged.

A note about lockfiles

Lockfiles are notoriously hard to review and make an excellent attack vector. A bad actor might attempt to add an “evil” dependency without anybody noticing. On the next run of dependency installation/updates, they could have access to the machine and everything on it.

If you are submitting a PR that contains a lockfile such as package-lock.json for Node.js or composer.lock for PHP, please explicitly explain why in the PR description.

If you are reviewing a PR that contains a lockfile , take extra caution before merging the PR. First, ensure the lockfile is explained in the PR description and the diff is readable. If not, do not merge it.

In either situation you should:

• Request the PR submitter to addresses why lockfile updates are necessary as part of the PR description
• Request the PR submitter to remove any file(s) with unreadable diffs and forward to pull request to your manager

For particularly complex lockfile updates, a senior developer or manager will often merge the PR and then update dependencies manually, controlling the lockfile changes themselves.

For brief and readable lockfile diffs, spot checking the “resolved” element for hostname URL changes is a best practice. While hostname file version changes are expected, hostname URL changes should be uncommon and confirmed as valid before merging.

Reviewing Dependabot PRs

Dependabot is a GitHub repository tool for automated dependency updates for security vulnerabilities. The changes will be reflected in the lockfile and should be perfectly safe to merge. Even still, it’s best practice to review the diff while considering the above note about checking the hostname URL for changes.

Review code against standards

When reviewing code, please ensure changes adhere to Git, code quality, and style best practices

Review content against standards

When creating or reviewing Markdown content, ensure technical documentation standards have been met.

Review functionality and user experience

When reviewing a functional or content change, both PR creator and reviewer should run the changes in a Codespace or pre-production environment, specifically looking for:

• Expected behavior of the proposed change
• Expected behavior of related functionality, surrounding the change
• A seamless and intuitive user experience

Pull requests that require testing, building, previewing UI elements etc. are checked out directly from the upstream repository (not the PR author’s fork) using GitHub CLI locally or in a devcontainer/Codespace.

gh pr checkout <PULL_REQUEST_ID>

For example, to checkout PR #300 in a Codespace:

1. Create (or open an existing Codespace) on the repository against which the PR was created.
2. Run gh pr checkout 300 in the Codespace’s terminal.

Squash related commits before merging pull requests

Selecting Squash and merge when merging a PR will squash all individual commits from that PR into a single commit.

Resolving pull request conflicts

While it isn’t a hard requirement, pull request conflicts should be resolved via rebase.

Requesting review of your pull request

As discussed in LinkORB’s asynchronous communications guide, use of @ notifications should be reserved for truly time-sensitive issues. Usually requesting review of a pull request does not warrant an @ notification.

Best practices to request review of your pull request include:

• If the repository contains a codeowners file, no further action is required. Code owners are automatically requested for review when someone opens a pull request that modifies code that they own.
• Create a subtask in your Team HQ work item, requesting PR review and assign it to the appropriate reviewer.

Use of draft pull requests

If you have an in-progress branch and want to see the compare on GitHub or get preliminary feedback from other team members, use GitHub’s draft pull request feature.

The PR author ensures all PR checks pass

The PR author is primarily responsible for ensuring that all PR checks pass. Instead of having to monitor/refresh a PR’s page to detect failing checks, the author should review GitHub related email notifications shortly after opening a PR or applying changes to the PR to catch and fix failing PR checks.

Requesting changes to a PR

Unless you only have a single comment, the GitHub review mechanism which bundles multiple comments into a single notification is preferred to adding multiple comments one-at-a-time.

After adding all your feedback, you can request changes to reassign back to the author. This flow works well on GitHub itself and includes comments in the card timeline (when the card number has been included in the PR title).

If changes are required and the Team HQ card is currently assigned to the PR reviewer and review status, update the assignee of the relevant Team HQ work card and return to sprint status.

No issue or question is too small

Often there is pressure to quickly merge pending changes and move on. However, at LinkORB we believe collaboration is a key to success. As such, no issue with or question about a PR is too small for discussion.

It is not to say a PR should be held up if a submitter did not follow commit conventions, however re-iterating the best practice to the submitter for future PRs is appropriate.