Code Reviews

Owned by Luka Mirosevic
Apr 20, 2020
2 min read

Status: DRAFT

Code Reviews

Summary

  • PR author assigns a principal reviewer

  • Once that reviewer is happy, he should merge the PR. by default

  • If the author doesn’t want a reviewer to auto-merge, then this needs to be explicitly specified

  • After merge: pls delete the feature branch

  • The assignee of the task is responsible for moving it into “done” on Jira (once the PR gets merged).

 

When to make a Code Review?

When you have some time after your current issue is done. Don't leave it right until the end, as that will block the PR author too long.

 

Who is responsible for asking about CR & merging?

  • Author of the PR must nominate a principal reviewer.

  • The principal reviewer has the authority to merge the feature.

  • Sometimes the author wants a preliminary review, even though the feature isn't done, in this case, the PR needs to be clearly marked as WIP.

  • The author is still responsible for keeping Jira issue up to date. That means when an author sees that the PR has been merged, he should transition the Jira issue to Done.

 

Time expectations

  • 0-48h. 48h is a maximum and we should try to make it faster, especially when it blocks other people's work.

  • If proper code reviews cannot be done due to time constraints, flag this to the squad TL.

 

How big can my PR be?

Effective LOC should be < 500. The larger PR, the longer and less useful is the review.

 

PR name

Please always include JIRA issue ticket key in the PR name, for example: [WEB-10] Add About page

 

What should be included, apart from the code?

  • Documentation updates if needed

  • (project-specific) Sane tests to cover real-life cases

  • (project-specific) E2E tests for critical paths

  • (project-specific) Storybook stories with examples for new Components

 

What should the Reviewer do?

  • No 5-seconds review - please take your time and investigate as much as possible; please keep in mind that by approving the PR, you agree with the quality of the work done!

  • Code checkout and clicking through the feature to find bugs is mandatory.

  • Verifying that the feature is exactly according to the specs is generally not the reviewer's job. That's the PM's job.

 

Shared components

If the project uses components from a shared library, and changes are made to this shared library, you must get approval from the TL and at least 1 person from each team that depends on the shared library.

 

 

Owner

Reviewer