Please follow these simple steps to make sure your PR gets enough attention, is reviewed and merged.
-
You have a good, informative and precise TITLE to tell exactly what your PR is about.
-
You have a great DESCRIPTION to describe what has been achieved in this PR.
- Refer to an open issue that gets resolved with this PR.
- Talk about the approach taken for the solution.
- Include images, screenshots and diagrams relevant to understand the PR better.
-
ASSIGN YOURSELF (and relevant people that were directly responsible for the solution) on the PR.
-
Add appropriate LABELS to the PR to make sure proper context is set.
-
Add the PR to appropriate PROJECT.
-
SELF REVIEW the PR before requesting anyone else for a review. Reviewer is going to be investing his time and energy into reviewing your code and giving you healthy feedback. Please make sure you respect that.
-
If the PR is ready for review by your peers, REQUEST AT LEAST ONE REVIEWER to review your code.
- If the PR is not reviewed by the reviewer in TWO WORKING DAYS, please mention the PR reviewers on Slack with A QUICK LINK to the PR.
-
DO NOT MERGE until you get at least ONE APPROVAL and all the change requests are resolved.
-
While merging the PR:
- MERGE COMMIT MESSAGE TITLE is PR TITLE.
- MERGE COMMIT MESSAGE DESCRIPTION is PR DESCRIPTION.
-
After merging the PR branch, make sure you DELETE THE BRANCH and not allow it to stale.
-
Once the PR is merged - mention this on Slack that the PR was merged and what change it has introduced in the code base. This is optional, but if the PR has made a significant change that is going to affect many, please do.
IMPORTANT NOTE: Be humble, generous, curious and respectful while you review someone's code. They have requested a review to help them learn and grow, and not because they want to show off their skills. Thanks!
-
Make sure the REQUESTER has followed the above checklist. If there is something missing, kindly mention that on the PR as a comment before reviewing and allow the REQUESTER to update the PR.
-
General Pointers:
- Is the APPROACH taken to solve the intended problem CORRECTLY IMPLEMENTED? Is this the best approach?
- Are the LOGIC and PROGRAMMING CONSTRUCTS correctly used?
- Does it have correct LINTING?
- Is there any REDUNDANT code?
- Are there any CODE SMELLS?
- Are there any DESIGN PATTERNS implemented? Are they implemented correctly?
- Is the code as MODULAR as possible?
- Are SOLID, DRY, YAGNI, etc software development principles followed?
- Are RACE CONDITIONS and BOUNDARY CONDITIONS handled correctly?
- Can any GLOBAL VARIABLES AND CONTEXT be replaced?
- Can any of the code be replaced with LIBRARY FUNCTIONS?
- Can any LOGGING AND DEBUGGING CODE be removed?
- Does the code follow DEFINED ARCHITECTURE?
- Does feature implementation follow USER ACCEPTANCE CRITERIA and specified design?
- Ensure PR passes CI server, CI IS GREEN, re-running once or twice if it fails (flaky tests)
- REQUEST CHANGES to the code and/or additional unit tests if any issues are found.
- Is any code invoking MEMORY LEAKS?
-
Readability, Documentation or PR explanation?
- Are ALL THE THINGS NAMED WELL?
- Is all the code easily understood?
- Is any edge-case handling described?
- Is the use and function of third-party libraries documented?
- Are data structures and units of measurement explained?
-
Testing Pointers:
- Is the CODE TESTABLE?
- Do tests exist and are they comprehensive? (i.e. code test coverage is as per agreements)
- Is TDD, BDD followed?
- Do unit tests actually test that the code is performing the intended functionality?
- Are arrays checked for out-of-bound errors?
- Could any test code be replaced with the use of an existing API?
-
Before finishing the review make sure your comments are WELL WRITTEN, WELL WORDED and ACTIONABLE.
-
FINISH THE REVIEW with a final CONCLUSIVE COMMENT.
-
If changes are requested, please check REQUEST CHANGES, otherwise, if PR is good, APPROVE.
-
ALLOW THE REQUESTER TO MERGE the PR, by explicitly mentioning about the merge go-ahead in the APPROVAL COMMENT.