Skip to content

Latest commit

 

History

History
68 lines (47 loc) · 3.41 KB

README.md

File metadata and controls

68 lines (47 loc) · 3.41 KB
Raw

Code review

  • Code reviews are everyone's responsibility
  • Reply to a code review request as soon as you can
  • Limit the number of reviewers (no more than 2)
  • Remember to praise
  • Reviews should be based on agreed-on coding standards, not personal preferences

Focus (mainly) on the logic rather than the formatting

Provide constructive feedback

  • Prefer questions to demands
    • Avoid assumptions, ask for clarification
  • Link to documentation, blog articles...

Does it have meaningful tests

  • Are the tests testing the behaviour rather than the implementation details?

Is documentation required

  • If it's a new service, could you start it up on your own?
  • If it's an integration with a 3rd party, could you run it on your own?

Consistency

  • Is the author re-inventing a pattern that is already present in the codebase?

Don't block the author

  • Once you're happy with the changes and you're confident the developer will address the remaining comments you can approve with suggestions so that the author can merge the changes without requiring any further interaction
  • Prefix comments with "Nit: " to indicate to the author that they're not expected to fix them
  • If you find some major design issues, provide feedback immediately rather than reviewing the whole Pull Request

Behaviours to avoid

  • Drive-by comment: leaving a single comment without following through or approving the changes, hence forcing the Pull Request author to contact you
  • Oral feedback: not capturing the feedback that led to changes, if it's not written it'll be forgotten

Conflict

Sometimes you go back and forth a few times on a specific comment without reaching an agreement:

  1. If possible discuss face to face
  2. If you still can't resolve the issue, involve a third person

References