-
Pull Requests should be small and easy to review.
-
Pull Requests should have a clean description and clean title, a helpful and structured description.
-
Rebase and merge is the preferred way to merge to main to achieve a clean and linear git commit history.
Pull Requests is a Github tool that provides us with the ability to better review code and comment on it. In addition provides a set of tools (actions/checks) that allows to automate checks of all kinds (ci) as well as produce artifacts that are relevant to the documentation, release and deployment of the related repository’s packages, services etc.
Github provides the compare endpoint that allows us to review changes between two branches. This is the base of a Pull Request and is very useful tool because it allows to share changes over any two commits(sha)/branches as a link before even opening a PR.
In addition, Github also allows for opening a PR in the Draft state. Draft is basically a fully opened PR that by convention is not ready for official review and/or merge.
Because our reusable workflows do not handle draft PRs at the moment, we avoid them because they kick-off the pipelines.
Is my PR ready for review?
It is important, for everyone’s time and especially for the reviewers' time, to make sure that your PR is ready before asking for review.
Here is a checklist for things to consider:
-
Do I provide a PR description? This is of course not required for extremely simple PRs, for sure. In most cases however you will want to have something there. See Below for details.
-
Do I have a clean commit history? While you are working on a feature, with or without having open a (draft) PR, you might make as many changes as you like. Create or revert commits, do fixups, rebase and force push etc. However, before asking for a review, make sure that all the commits presented for review are only the ones related to the actual feature. No temp commits, revert commits, fixups etc. Rather, a clean history. See below “Example of managing a feature branch”.
-
Have I reviewed my own PR? Always do a self-review before asking another person to review. You’d be surprised how many small things here and there you will catch. This is very positive for two reasons. First, it makes a better impression to the reviewer. Second, the reviewer focuses more on the essence of the change.
Let’s say we are working on a feature branch and the base branch is main.
git checkout -b feature/ticket/name
# make some code changes, commit and push
git add
git commit
git push
# make some more code changes, commit and push
git add
git commit
git push
# you want to correct something that belongs to the first commit
git add
git commit --fixup sha-of-first-commit
git push
# at this point you have 3 commits, 1 of which is a fixup to the first commit
# your feature is now ready
# before asking for review, finalize your branch
# pull latest `main` and come back to your feature branch
git checkout main && git pull && git co -
# squash any fixups with interactive rebase
git rebase -i --autosquash main
# now your branch is ready
# it should have 2 commits
# at this point you need to force push
git push -f
# prepare the PR and ask for review
When opening a new PR, if the diff between the base and the feature branch is a single commit, Github by default will populate the PR title and description with the commit’s title and description.
The PR title will probably need some formatting to be more readable. For example, the commit title consists of some parts like the conventional commits context (feat, fix etc) and the Jira ticket, both of which do not make much sense in a PR title.
It makes sense to always include a link to the related Jira ticket in the PR description to make navigation back and forth easier for the reviewer.
We propose the following structure for the PR descriptions:
## Description
## Ticket link(if appropriate)
## Screenshots (if appropriate)
## Checklist before requesting a review
- [ ] I have performed a self-review of my code
- [ ] I have added thorough tests
- [ ] The commit messages follow our guidelines
- [ ] There are no linting errors
Consider adding notes to the reviewers as well if applicable and point out any other caveats that apply to the PR as a whole, for example, dependencies to other PRs that need to be merged before this one etc.
Questions to ask yourself:
-
What are the problems of the current implementation?
-
What are the benefits of this change? Some examples?
-
Are there any other side-benefits?
-
Are we incorporating breaking changes or do we keep backwards compatibility?
-
Does it change the current api? If yes, how?
-
Does it change how the package/service is used?
-
Does it have any other side effects like effects in other packages that use it implicitly?
-
Do we need change the patch, minor or major version?
The size of the PR matters. The smallest the PR the better.
Smallest PRs are easier to review and as a result it is easier to catch errors, bugs, security issues etc. It is easier for the reviewer to build a mental model of the changes and the possible side effects.
There is no specific rule of what constitutes a very large PR, but it is something that you will get as a feeling when reviewing your code yourself.
You should consider splitting one PR to multiple PRs if the code changes are more than a few tenths.
For this process to be more effective, it is important to have small and meaningful commits, that sit well on top of each other, each with clear context. This way, if for any reason a PR gets too big, it will be much easier to split, by creating a new branch and cherry picking commits from one branch to another. One to be reviewed now, one to be reviewed after the original has been merged.
There are exceptions of course to this rule, but are more rare and should be discussed ad-hoc by the responsible team.
In case you split your PR into two or more, or in case you want to work on a feature that is based on a branch other than main, Github supports changing the base branch for your PR.
For example, you want to start working on a task B that is dependent on another task A that is not yet merged but is awaiting a review.
You can create a new branch B from that feature branch A, push it to Github and create a the PR with branch A as the base branch.
When branch A gets merged to main, Github will automatically change the base branch back to main.
This is different to Pull Requests on environment env/* branches. See more here.
When reviewing code we should follow a set of general guidelines in order to have consistently good outcomes, good collaboration, communication and understanding, catch errors etc.
When it comes to communication, when making comments:
-
Be polite
-
Refer to our rules (i.e G00001) if applicable. These references contain an explanation with examples, motivation etc.
-
Try to provide other references
-
Try to provide solutions or examples
-
Assume good intentions
There are several kinds of comments:
-
Questions ❓ :question
-
Requests for modifications/fixes ✂️ :scissors 🪛 :screwdriver
-
Nice to have suggestions 🙃 :upside_down_face
-
Alternative implementations suggestions 🪛 :screwdriver 🙃 :upside_down_face
-
Caveat , beware ❗ :exclamation
-
Good job 💯 :100 👍 :+1
Ideally, commits should be reviewed one by one. The reason is that there is no other way to review the commit itself (a commit message might be misleading, grammatically or syntactically wrong etc).
Github provides a way to easily navigate back and forth between commits. By clicking on the first commit for example and then using Prev/Next links:
However, there are many people using VS Code’s Github Extension to review Pull Requests directly in VS Code. The extension does not provide a way to review commits separately. In this case, the reviewer is responsible for figuring out the best approach depending on the case.
Ticket/PR owners are responsible for letting reviewers know about the PR readiness.
By convention we do not rely on GitHub reviewers selection section and notification.
Rather we:
-
Move the ticket that is under review in the To be reviewed column on the Jira board
-
Change the ticket Assignee to the reviewer
-
Politely let the reviewer(s) know about that
- If you feel it can be reviewed quickly, this could be a chat. Alternatively, a short mention during the standup “Oh yeah, I put ticket x up for review for you” is also a good way of communicating.
From that point on, the (remote) branch of the PR should be considered locked any no more changes should be pushed until the reviewer is done reviewing. Mind that this might take a while and you should not assume that a review in done unless being explicitly so.
By convention again, when the reviewer is done:
-
He will move the ticket back to the In progress column on the Jira board
-
Change the ticket Assignee back to the ticket owner.
In general, we aim to have only 1 mandatory back-and-forth. However, we do make exceptions if:
-
a) requested by the reviewer or
-
b) wanted by the owner
Last, a ticket is considered Done, in which case:
-
The owner is responsible for moving it over to the Done column on the Jira board
-
Merging the related branch and closing the PR.
Github provides us with the ability to comment on a specific line or a group of lines.
You can comment on group of lines by clicking on a line and dragging the mouse pointer up or down.
In addition, it allows the comments to be Resolved.
By convention, an unresolved comment needs attention and a resolved comment is already looked at and fixed.
When a reviewer makes a comment:
-
The owner of the PR can acknowledge it by adding a 👍
-
If the comment triggers an action, when this action is implemented, the owner is responsible for resolving the comment/conversation. Otherwise, resolve instantly.
Try to not leave comments unresolved because it will feel like the PR has still things to be done, it will look unfinished.
There are 3 merge strategies:
-
Squash and merge
-
Merge commit
-
Rebase and merge
We will not talk about Squash and merge because we do not allow it. There are several companies that follow this workflow, including Biostrand up to a point.
Squashing commits via Github is a terrible idea when you want to have a clean, structured, helpful and informative git commit history.
Squashing, if required should happen locally with interactive rebase, before pushing to the origin branch.
Merging with a Merge commit makes sense in some cases but it should be used with care.
You have to remember to rebase your branch to main before merging, even with a merge commit. Otherwise the git history will not be linear.
Use a merge commit if you branch consists of more than one commit and it makes sense to group them logically under an “umbrella” of a merge commit.
In most cases, simple rebasing to the main branch and merging is sufficient.
This is especially true, if you follow best practices and your PR consists of one small and concise commit.