Skip to content

Engineering Team Norms

Jump to bottom
Hannes Schmidt edited this page Jan 29, 2018 · 8 revisions

Here's some basics about how we agree to work together:

  • We will be on Slack during working hours, which is roughly 9am - 5pm PT Monday thru Friday
  • We will use Slack as our primary means of communication, using the #data-store-eng channel
  • We will strive for 24 hour review turnarounds during the work week for pull requests
  • We will have 2-week sprints and use Github Project boards to manage our tickets. See Sprints and Standups.

Pull Requests & Code Reviews

  1. Before asking for a review on a PR, make sure that it is all green. You may ask for a preliminary review of a work in progress. If you do so, make sure the PR title starts with [WIP]. When reviewing a work in progress, don't approve the PR, either reject it or make comments.

  2. When authoring a PR, don't nickel-and-dime the reviewer. Put the PR into a shape that you think is ready to be merged. Then ask for reviews. Don't modify a PR on which reviews have been pending for less than eight business hours.

  3. Request reviews from at least one team member.

  4. Before merging a PR make sure that it

    • was authored by you
    • does not have any rejections
    • is all green
    • has no unanswered review questions
    • has only change requests that have either been
      • withdrawn by the reviewer or
      • addressed by the author

  5. While it is acceptable to massage the history (rebasing, squashing commits) after a PR has been accepted, it is not acceptable to alter essential functional aspects of the change.

  6. When you are being asked to review a PR, please do so promptly. Being agile means unblocking others.

  7. When reviewing a PR give it your full attention. Review the entire diff unless you find a "show stopping" issue, a fix for which would render the remainder of the diff obsolete. Don't nickel-and-dime the author (don't stop reviewing at the first question or issue that comes up during the review).

  8. Reasons for rejecting a change include:

    • it is functionally deficient (it doesn't work)
    • it's brittle (an unrelated change could cause it to break)
    • it doesn't have enough tests
    • it's insufficiently documented
    • it's in violation of the code quality standards formally accepted by the team

  9. When requesting changes, be respectful and provide a reason for each change requested.

  10. When chaining PRs start the PR title with a call sign identifying the chain, a numeric ordinal and the total number of PR in the chain. For example:

    • Reindexing: 1/3: Refactor indexer to accommodate REPAIR and VERFIY
    • Reindexing: 2/3: Implement VERIFY
    • Reindexing: 3/3: Implement REPAIR
Clone this wiki locally