Code is never merged directly to the
master branch. Each feature, bug or chore branch requires a code review.
Guidelines for authors
- Submit a pull request for the branch that needs to be reviewed
- The name of the pull request must contain both the user story ID and title:
[<User Story ID>] As a user I can view the list of job positions
- The description of the pull request must contain the link to the user story to make it easier for reviewers to get the complete requirements e.g. https://www.pivotaltracker.com/story/show/133252875.
- At least two reviewers should be added to each pull request: either two team members in the project or, for a smaller project, one team member in the project and one “outsider” (not working in the project).
- Reviewers approves the pull request when it passes the team guidelines and solves the issue at hand in a satisfying way.
- Rebase your local branch before submitting your pull request to make it ready for merge.
- Do not wait to complete all requirements before opening a pull request. In this cases, append the suffix
[WIP]to the pull request title.
[<User Story ID>] As a user I view the list of job positions [WIP]
Work in progress pull requests are encouraged to both get early feedback on an implementation and share it with the team. For instance, other team members could cherry-pick code from the branch even though it is not complete.
- Seek to understand the reviewer’s perspective. Assume the best intention from the reviewer’s comments.
- Be grateful for the reviewer’s suggestions e.g.
Good catch. I will make that change.
- Respond to every comment either offline (in person) or online.
- Push commits based on rounds of feedback as isolated commits to the branch. As good practice reply to the comment with a link to fix e.g.
Fixed in a4994ec
- Extract some changes and refactorings into future tickets/stories if the requested changes are large and/or out of the initial scope of the PR.
- Merging a pull request can only happen if both reviewers approved it and it passes the tests suite.
* Assign the appropriate labels to the PR. Labels are used to categorize the type of PR (
chore…) and priority (
* Assign a milestone to the PR. Usually, a milestone corresponds to a release version.
Guidelines for reviewers
- Accept that many programming decisions are opinions. Engage a discussion and reach a resolution quickly.
- Comment directly on the line of code in the web interface of the version control service (Github, Bitbucket or Gitlab)
- Use an inquisitive tone, do not make an order:
# Bad `@user_id` -> Change this variable name to "NEW_VARIABLE_NAME" # Good `@user_id` -> What do you think about naming this "NEW_VARIABLE_NAME"?
- Ask for clarification e.g.
I didn't understand. Can you please explain?
- Avoid selective ownership of code. (
- Seek to understand the author’s perspective.
- Identify ways to simplify the code while still solving the problem.
- PRs should be reviewed within one day. But it’s ok for PR authors to ask for a speedier code review on Slack
- PRs should be merged within 5 days i.e. during the current sprint or next sprint at most. Stale PRs are usually hard to merge past this time range and require difficult rebase.
- Approve the PR by using the review tool of version control service (Github, Bitbucket or Gitlab)
- Merge once you feel confident in the code and its impact on the project.