Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

...

  1. Is there a JIRA card created with description of the feature/issue and acceptance criteria articulated in the card?
  2. Is the PR marked with the right JIRA card number? When you merge, you must ensure that the JIRA card number is reflected in the commit msg.
  3. Are you the right person to review this PR? If not, try to direct to the right person. If you have partial context, try to find help from one who knows better.
  4. Are any checks (travis-ci, codeclimate) failing? If so, immediately request that the contributor look into this.
  5. Has the submitter signed the CLA? (See the status of the cla-assistant check.) If not, kindly ask them to do so.
  6. Ask questions, instead of making statements. Avoid “why” - which often seem accusatory, rather try to understand the context and reasoning … “what was the reason for ….”
  7. For bigger PRs, try to “meet” with the person. Often pairing on the PR review for big PRs is the best way. Bigger PRs are also best to merge onto a branch, run/check locally and then commit.
    1. You can follow github ‘Command line instructions’ suggestions to merge a PR locally before pushing onto master. Often for large PRs and in cases you can’t assert the correctness of the code without running it locally, this is a preferred approach. Just go the PR link in github; towards the end click “command line instructions” next to the “merge pull request” button and follow the suggestions.  
  8. Lots of handy tools exists for code analysis - e.g. a Static Code analyzer with IntelliJ or Sublime

...

  1. Code reviews are a discussion not ‘dictation’/“orders”.
  2. The reviewer is equally responsible to the contributor - for correctness, design, security, readability and maintainability. Do not merge, and feel free to push back if you (reviewer) are not satisfied.
  3. It's fine if the contributor disagrees with your opinion, as long as he/she provides explanation that is logical.
  4. Direct the contributor to relevant code sections as examples to follow.
  5. Are there multiple commits that make it difficult to review or test locally? You may ask the contributor to squash into a single one, but remember that GitHub lets you "squash and merge", so you don't normally need to request this.
  6. Ensure there are corresponding tests for the PR relevant code
    1. Unit tests and Integration tests (if applicable) should be present.
  7. Does the code change something in the product that needs documentation (new or updates) in WIKI? - If so:
    1. mark the JIRA card accordingly by adding a "needs-doc-update" label
    2. ask the contributor to add the documentation (and verify that they include appropriate information like ‘applicable from version X’)
  8. Dependencies:
    1. Either to a library - ensure that the library is a well known and supported/latest one, is appropriate for the job, and there is no duplication (e.g. No Joda datetime library if it can be solved by Java 8 date APIs).
    2. To another module: Is the other module released? Discourage snapshot release dependency. In exceptional cases such snapshot dependency maybe allowed but ensure there is a JIRA card existence to resolve such dependency.
    3. Obvious: no circular dependency of components/services/codes
  9. If you (reviewer) discover a missing “acceptance criteria”, please update the JIRA card.

Others

  1. Has the contributor signed the ‘Contributor License Agreement’? Kindly ask for it if not done. (TO DO: link to CLA)

What to Review?

See JIRA for cards that need code review in the next release:

...