Skip to content

Engineering Team Norms

Jump to bottom
Hannes Schmidt edited this page Jan 27, 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

  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 two team-members. Chose reviewers that are familiar with that part of the code base.

  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

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.

It is acceptable to merge a PR with only one approval and a review that has been pending for more than eight business hours.

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

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).

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

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

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