Skip to content

Pull Request Principles

To help streamline the development workflow its best practice to standardise PR creation, as the team grows this helps to ensure PRs are quickly reviewed, are traceable, and accurately describes the changes for clarity and versioning tools.

Creation Principles

Keep it small, where possible

  • Keep PRs focused and small to facilitate easier review and integration. This helps ensure branches are short-lived and minimises conflicts.

Make it Traceable

  • Prefix the PR name with the ticket number to maintain traceability.

Describe, Conventionally

  • Include a detailed description of the changes made, adhering to the Conventional Commits guidelines. This is important as we use a Semantic Versioning tool which uses this description to version the code.

Prefix commits with one of the following labels:

Type Description
build Changes that affect the build system or external dependencies (e.g., gulp, broccoli, npm).
ci Changes to CI configuration files and scripts (e.g., Travis, Circle, BrowserStack, SauceLabs).
docs Documentation-only changes.
feat A new feature.
fix A bug fix.
perf A code change that improves performance.
refactor A code change that neither fixes a bug nor adds a feature.
style Changes that do not affect the meaning of the code (e.g., whitespace, formatting, semicolons).
test Adding missing tests or correcting existing tests.

Warning

Remember to use an exclamation point (!) when the change is breaking.

Create as Draft

  • First create the Pull Request in the draft state, this stops a notification being passed to the team before the PR is ready for review.

Tidy & Polish

  • Resolve any issues raised by automated review tools & pipelines. Ensure automated quality gates are passing before marking ready to review.

Reviewer Assignment

  • Set the reviewer to the software engineers group to allow any team member to pick up the review.

Discuss

  • Optionally, discuss the PR on the thread created in slack by GitHub. This is a good idea as it keeps the discussion traceable and keeps the whole team in the loop.

Review Principles

Engineer Priorities

Reviewing another team member's PR is a high priority, aim to review PRs as fast a possible to help get the near ready code shipped. Mostly, it will make sense to stop your current work to submit a code review.

Review Context

  • Before you start to review the code on a PR, make sure you understand the intent of the change, this should be easily understood from the description, comments on the PR or discussion on the ticket the PR relates to.

  • Check to see if the automated quality gates have passed

How to Comment?

Using a modified version of Conventional Comments

Prefix comments with one of the following labels:

Type Description
praise Highlights positive aspects. Try to leave at least one per review. Must be sincere, not false.
nit Non-blocking comments - including educational remarks, context sharing, suggestions, aspirations
suggestion Proposes significant improvements with explicit explanations of what and why.
issue Highlights specific problems, user-facing or internal. Best paired with suggestions.
todo Small, trivial but necessary changes, distinct from issues/suggestions.
question For potential concerns needing clarification or investigation.
chore Required tasks before acceptance, usually referencing common processes. Include process links.
note Non-blocking comments highlighting important information.
typo Spelling error - Use in combination with a code suggestion to actually help the author

When to Request Changes?

File Relevancy

  • Make sure all files included in the PR are relevant to the context. Ensure no files have been accidentally added.

Coding Standards

  • Ensure that the code standards are met, however raise an issue with the automated tools & linting rules where automating the detection in future is possible.

Are we testing the right things?

  • Make sure the test cases are actually testing the newly added logic in a realistic manner.
  • Are realistic edge cases covered?

Logical Improvements

  • Can you spot a potential bug in the logic?
  • Can a significant performance improvement suggestion be made?

Security

  • It's important to ensure that no vulnerabilities have been introduced by the changes

If any comments exist for the above sections, changes should be requested

Nit Picking

  • Using the definition of a Nit as: *non-blocking comments - including educational remarks, context sharing, suggestions, aspirations,
  • ✅ Providing educational remarks and additional research context
  • ✅ Briefly outlining an aspiration for future PRs
  • ❌ Making trivial preference based comments, as the code standards should outline what is the agreed team-wide practice, if missing from standards suggest an amendment to the code standards to the team.

Nits are review comments that don't expect a change to be made by the author. Use only when helpful & aim to minimise cognitive load on the author.

Code Suggestions

GitHub allows us to make code suggestions on comments by following this guide. Using this allows us to collaborate on a PR and help the author ship faster. Use this wherever possible but especially on issues like typo's where the code suggestion is essentially the same amount of work as the comment.

Further Reading