diff --git a/pull-requests.md b/pull-requests.md index f9a7b9d..005bf6a 100644 --- a/pull-requests.md +++ b/pull-requests.md @@ -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.