Discussed and agreed by all Data Hub developers. If anything changes, please keep this document up-to-date and in sync with the related backend/frontend one.
- Make sure your code is appropriately documented and includes change log fragments describing the change
- Keep the overall coding style consistent with the rest of the repository unless it's been identified as a particular style that we want to move away from. Avoid using your own style as this can cause disagreements among developers. When adopting a new agreed style (ideally) refactor the existing code to keep the overall codebase consistent
- Follow Uncle Bob's boy-scout rule: “always leave the code behind in a better state than you found it”
- Include unit, functional, e2e and compatibility tests (when applicable)
- Make sure CI build passes consistently without any flaky tests
- Follow branch naming conventions by prefixing your branch name with feature/, bugfix/, removal/, test/, hotfix/ or release/
- Make sure commits are logical and atomic - each commit should include tests
- Keep each commit small and deployable - each commit should ideally leave the main branch in a releasable state
- Use imperative mood in commit message but it's okay to use past tense in the description
- Make sure your PR is atomic and doesn't solve multiple problems at the same time
- Keep your PR small and deployable - each PR must leave the main branch in a releasable state
- PRs shouldn't normally add or change more than 400 lines of code at the time
- Use feature flags if your PR cannot be deployed to production at any time after being merged
- Alternatively hide a new piece of functionality behind an express.js route being careful not to expose the URL
- Use GitHub labels if your PR is blocked or depends on another
- Use a GitHub draft PR for WIP or if you want initial feedback
- Document what the PR does and why the change is needed
- Give full context - not everyone has access to Jira/Trello
- Detail anything that is out of scope and will be covered by future PRs
- Include details on the lifecycle of the feature and its nature. Is it a new feature or a change to an existing one? Is the code going to be short-lived? Is it part of a bigger piece of work?
- Highlight possible controversies
- Include instructions on how to test (e.g. what should I see?)
- Detail any considerations when releasing
- Add before / after screenshots or GIFs
- Include screenshots of both mobile and desktop versions
- Remember that you are starting a conversation
- Don't take feedback personally
- Be honest, but don't be rude
- Non-trivial questions and issues are best discussed via Slack or face-to-face
- The same applies to PRs with large number of comments
- At the end of a conversation, update GitHub with a summary
- Make sure you read and follow the guidelines in Step 1: before and when submitting a PR
- Don't
rebaseorpush --forcewhilst the PR is being reviewed otherwise reviewers won't be able to see what's changed - Don't dismiss change requests except in rare circumstances (e.g. when the reviewer is on holiday), document the reason
- Mark commits that will be squashed into other commits with the fixup: prefix. This helps reviewers understand how you are planning to organise your PR after approval
- Respond to comments if you don't agree with them
- If you decide not to implement the suggestion, come to a mutual understanding with the reviewer
- Allocate 30 minutes during the day for reviewing PRs
- If there are multiple PRs, the preferred method for selection is FIFO (first in first out). However, this could result in fewer PRs being reviewed by multiple developers, so use common sense
- Whenever possible, review a PR in one go so that the author understands the amount of work needed and can plan with his/her team
- Aim to spend no longer than twenty or thirty minutes per Pull Request. For sure we are looking for a reviewer to immerse themselves in the sense of the Pull Request but it's not always necessary to check out the branch and test it yourself
- The purpose of the code review is to ascertain whether or not the Pull Request is good enough. Not perfect but good enough. Think of it like "proof-reading" a written document. Of course PRs are an opportunity to share knowledge and technique, but if any of your comments fall into this category, please mark them as "Optional" in the comment body - see section Levels of Importance below.
Bear in mind that when receiving feedback as an author of a PR, it's easy to feel overwhelmed in the face of a high volume of feedback. This is especially true if you receive a bunch of comments from several peers, then a little later on one of those reviewers adds some more points. It can feel like there's no end to it.
So for these reasons you ought to observe the following rules when reviewing a PR:
-
Use the GitHub "start a review" feature when commenting. Don't just write "single comments" - bundle all your comments up into one batch which is shared with the author only when you finally submit your review as "Comment, Approve or Reject". This means that the author of the review can be safe in the knowledge that your input has concluded and that they have a complete picture of your thoughts on the PR.
-
You only get one shot at a review. You should see your involvement as a single, one-off interaction with that request. Once you've submitted it your only future involvement in the PR should be to respond to / resolve points that you're already raised. If you think of something incredibly important after you've submitted your review then you need to contact the author directly and pick this point up with them as a personal conversation.
- A Pull Request is not the forum for architectural review. Once work is at such a level of maturity that it exists in a PR, all the architectural considerations should have been handled already. If work contains anything particularly unusual in terms of its approach, that should have been discussed at one of the weekly Developer Huddles, well in advance of starting work on the actual code.
- Offer suggestions - “It might be easier to...”, “Consider...”
- Be objective - “this method is missing a docstring” instead of “you forgot to write a docstring”
- Feel free to constructively challenge approaches and solutions, even when coming from a seasoned developer
- It's OK to nit-pick on syntax issues, spelling errors, poor variable/function names, missing corner cases
- But don't be a perfectionist - allow some flexibility
- Prefix non-critical comments with Optional: so that the author knows what is important and what is not
- If all your comments are non-critical, leave your feedback but accept the PR at the same time so that you are not a blocker and you keep a positive attitude
rebase mainbefore merging to keep the history clean- Squash the fixup commits into their related ones