Creating and reviewing pull requests in [#git]

Pull requests are a critical step in the software development lifecycle.

Creating or reviewing a pull request (PR) of other teammates is an important part of working at LinkORB. While the team is always looking to automate adherence to styling and quality standards through the use of 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 description are brief, yet complete

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.

Note: By including the card number in the PR title, an automated process will add a link to the PR in the card properties as well as add GitHub PR discussion to the card history.

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

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.

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.
• Grant reviewers access to your fork of the repository in case they need to test changes in Codespaces with the following steps:
  1. Navigate to the main page of your fork and click the Settings tab.
  2. Click Collaborators and teams in the left side bar.
  3. Confirm that you have access to the repository through your preferred authentication method.
  4. Click Add people in the Manage access section.
  5. Search for the reviewer by name or GitHub username.
  6. Select the reviewer from the list of results for your search.
  7. Set the reviewer’s role to Read.
  8. Click Add reviewer’s name to this repository.

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.

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.