Make sure that your full name is displayed on the GitHub account you use for developing. Without it, it can be difficult to map authors back to real people.
If you’re not comfortable having your full name on your personal GitHub profile, please create a separate GitHub profile to use for work and add your name to that.
When first opening a pull request, use GitHub’s ‘draft’ feature to create it as a draft. This will prevent code owners from being tagged prematurely.
Only once you feel it is potentially production-ready, mark it as ‘ready for review’. This will then automatically tag the code owners.
If you want to get an early review while a pull request is still in draft, that’s fine.
By default, wait for the test suite to pass before asking for a review. One of main purposes of the tests (in particular, the linting) is to save a reviewer’s time by picking up on issues that would otherwise be flagged by a human.
Sometimes unrelated tests ‘flake’. Run the Ruff workflow before requesting a review.
Install and configure:
The only exceptions to requesting a review with failing tests should be when:
In either case, mention in your pull request description why there are failing tests.
Avoid big pull requests. They are:
There’s no hard-and-fast rule as to what constitutes too big a pull request, but if it’s running to more than a few hundred lines of code then it’s probably worth breaking up. If there are dependencies from one PR to another, chain them. (If you’ve made each commit atomic, this should be easy: you can just move later commits to another PR.)
Don’t apologise about a pull request being too big; break it up before seeking review. It is time well spent.
It’s never too late to break up a PR. If one becomes unexpectedly big or difficult during the review process, don’t soldier on: it quickly becomes demoralizing for everyone. Instead, identify smaller parts, break them out into separate pull requests, and focus on getting them merged incrementally.
Write imperative commit subjects, e.g. “Fix bug” rather than “Fixed bug” or
“Fixes bug.” This convention matches up with commit messages generated by
commands like git merge and git revert.
Tip: if you’ve done it correctly, the commit subject will complete the sentence, “If merged, this commit will…”
Add a proper name to the Pull Request. The name should be descriptive and should give a clear idea of what the PR is about.
Additionally add the Jira ticket ID in the PR title or description. This helps in tracking the PRs and also helps in understanding the context of the PR.
Best practice is to include Environment name in the PR title if the PR is related to a specific environment. Plus the Jira ticket id followed by description.
For example: PROD: [Jira_ID] Brief description of change or SIT: [Jira_ID] Brief description of change
Express a single thought with each commit. This can result in very small commits (e.g. fixing a typo) or quite large ones (e.g. moving many modules to different package). The point is that a reviewer should be able to hold the commit in their head without too much trouble.
What constitutes ‘a single thought’ is a judgement call. You may choose to add several interdependent Django models in a single commit, as it makes sense to be reviewed as a single thing: in this case, the data model. You may choose to use one commit to add type annotations for an entire module, because it is ultimately driven by one thought. The golden rule is that it should make the life of the reviewer easier.
If you’re writing ‘change this and that’ in a commit message, it’s a strong indication that you should break up the commit.
It’s hard to review commits that contain both refactoring (changes to the way code is expressed) and functional changes. Split them up so refactoring stands on its own.
Before a pull request merges, its commits should be self-contained (a.k.a. “atomic”).
This means that after each commit:
So don’t half-implement a feature in one commit then fix it in a later commit. Structure your pull requests so that each change keeps the codebase in a deployable state.
Tip: if you want to write a failing test and then fix it in a later commit, you can keep the test suite passing by
decorating the test with pytest.mark.xfail.
If you have a time-sensitive pull request that you are preparing, engage with your intended reviewers ahead of time to let them know it’s coming.
Complex PRs can take a lot of time to review: if you need a fast turnaround, the more notice the better.
Always respond to every comment to indicate to the reviewer that you’ve seen and thought about what they said. This doesn’t have to be onerous: often a 👍 reaction is sufficient.
When re-requesting a review after some requested changes, make it clear to the reviewer what has changed since their last review. This helps them save time as they may not remember where they reviewed up to last time.
There are a few approaches for doing this:
One way to address changes is to amend previous commits.
If you’re doing this it can force the reviewer to look back and reread lots of code they have already reviewed, so help them out by communicating each change with a PR comment.
Alternatively, you can make changes by adding commits on top of what you have already done, leaving the original commits in place. You can, if you like, use [fixup commits] to do this. If you do this, it’s helpful also to let the reviewer know which commit to review from.
Additional commits make changes clearer to the reviewer, but it has the downside of a messier commit history which will need to be tidied up before merge.
Don’t forget to squash the corrections together before merging, as per Clean up mistakes by rewriting history. You can add the Fixup before merging label to your PR to remind yourself.