Skip to content

Commit

Permalink
Merge pull request #164 from guardian/mc-improve-PR-recommendations
Browse files Browse the repository at this point in the history
Specify that PR can combine multiple changes, especially for dependencies upgrade
  • Loading branch information
mchv authored Apr 17, 2024
2 parents c191481 + af81509 commit 18441d3
Showing 1 changed file with 8 additions and 3 deletions.
11 changes: 8 additions & 3 deletions pull-requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,18 @@ Read over your code before opening the PR for review. The main point of code rev
Tip: Github allows you to open a PR in `draft` state, which can often be helpful if you want to review your own code, or have remote CI checks run, before others review it. (Remember to [change the status of your PR](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-stage-of-a-pull-request#marking-a-pull-request-as-ready-for-review) to `ready for review` later on though!)

## Small
A PR should be clear, and address a single concern.

There should be a manageable number of changes in a PR. Try to make your PR as small as possible while still advancing the functionality of the product.
A PR should be clear, address a single concern, and composed of a manageable number of changes.

If a PR is too complex, it is much easier for issues to be missed. In addition if trying to understand an aspect of the code change in the future, it becomes harder to understand if buried in a very large and complex PR.

When upgrading dependencies, if not [automated](https://github.com/guardian/recommendations/blob/main/scala.md#continuous-dependency-management), try to update several of them at the same time:
- For Scala, upgrade `scala` runtime, `sbt`, `sbt-plugins` and optionally `play-framework` all in one PR.
- For Node, upgrade `typescript` runtime, package manager, code formatter, linter, bundler, all in one PR.

Aim to deploy a PR before submitting the next one. Frequently small deployments of functionality are easier to review and easier to understand in PROD.
You are more likely to encounter incompatibilities or issues otherwise.

In general you don't need to make your PR as small as possible, and favour pace and frequency of delivery over atomicity of changes. Contrary to commits which can be [bisected to find root cause of an issue](https://www.metaltoad.com/blog/beginners-guide-git-bisect-process-elimination), every deployment will integrate additional changes in the system to the ones in the PR (for example baked AMI may be a new one, EC2 bootstrap script may install a newer version of system library, etc.) so you will be more likely to identify the reason for an issue through tests and monitoring than size of the PR.

## Releasable
A PR should be releasable once its review is complete.
Expand Down

0 comments on commit 18441d3

Please sign in to comment.