Skip to content

Latest commit

 

History

History
111 lines (85 loc) · 5.81 KB

pull-request.md

File metadata and controls

111 lines (85 loc) · 5.81 KB
Raw

Pull Request (PR) Guidelines

Submitter Guidelines

  • Title the PR using customer-focused language. The automatic release notes are generated from PR titles, so the titles should describe the feature from the user's perspective and avoid implementation details. For example, instead of "Add debug boolean", write "Support configurable debug mode". If the PR introduces a change that affects users and needs to be mentioned in the curated release notes, add a brief note that summarizes the change in the release-note block in the PR description.

  • Fill in our pull request template.

  • Make sure to create an issue and include the issue number in the PR description to automatically close the issue when the PR is merged. See Linking a pull request to an issue for details. An issue might not be required for PRs addressing typos, broken links, etc.

  • For significant changes, break your changes into a logical series of smaller commits. By approaching the changes incrementally, it becomes easier to comprehend and review the code.

  • Use your best judgement when resolving comments. Simple, unambiguous comments should be resolved by the submitter, this prioritizes speed and efficacy. For larger changes, for example, when updating logic and design, the resolution should be left for the reviewer’s approval or continued discussion.

  • If a discussion grows too long - more than a handful (3 - 5) back and forth responses, consider moving the discussion to a more real-time platform: Slack, Zoom, Phone, In-person. Do this to clear misunderstandings or whiteboard improvements. Whenever the conversation has moved to a different platform and found a conclusion, the decision and resolution MUST be updated in the review, for instance,

    Comment:

Spoke offline, decided to move A to package B. Rewrite tests and API docs.

  • When pushing code review changes, it's important to provide clear information to the reviewer. Here are a couple of guidelines to follow:

    • Commit each code review change individually.
    • Use descriptive commit messages that accurately describe the change made, making it easy for the reviewer to understand the purpose of the change.

  • When approved, and all review comments have been resolved; squash commits to a single commit. Follow the Commit Message Format guidelines when writing the final commit.

Reviewer Guidelines

  • As a reviewer, you bear the ultimate responsibility for the resolution of the comment. If not resolved, please follow-up in a timely manner with acceptance of the solution and explanation.
  • Do your best to review in a timely manner. However, code reviews are time-consuming; maximize their benefit by focusing on what’s highest value. This code review pyramid outlines a reasonable shorthand: Code Review Pyramid
  • Always review for: design, API semantics, DRY and maintainability practices, bugs and quality, efficiency and correctness.
  • Code review checklist:
    • Do any changes need to made in documentation?
      • Do public docs need updating?
      • Have internal workflow and informational docs been impacted?
    • Are there unit tests for the change?
      • If the change is a bug fix, has a unit test been added or modified that catches the bug? All bug fixes should be reproduced with a unit test before submitting any code. Then the code should be fixed so that the unit test passes.
      • If the change is a feature or new functionality. The expectation is to include a reasonable set of unit tests which exercise positive and negative cases.
    • Is this change backwards compatible? Is it causing breaking behavior?
    • Are there any Comment Tags in the change?

Commit Message Format

The commit message for your final commit should use a consistent format:

<One line summary of the change>

Problem: Brief description of the problem. The “why”.

Solution: Detailed description of the solution. Try to refrain from pseudo-code. Describe some of the analysis, thought
process, and outline what was done. Think of a newcomer coming across this two years hence.

Testing (optional): Description of tests that were run to exercise the solutions (unit tests, system tests,
reproductions, etc.)

Here's an example:

Make event stream handle large events

Problem: The event stream would choke on events larger than 1K. This caused events to be silently dropped, and affected
all downstream consumers.

Solution: Removed upper limit on event size. Also, added an error handler to warn when events are being dropped for any
reason.

Added unit tests for events up to 1MB.

Note Do not put any customer identifying information in message. Say “a customer found…”, NOT “ACME Corp found…”. If customer generated data is included it must be redacted for any PII (or identifying information). This includes IPs, ports, names of applications and objects, and URL paths. If customers volunteered this information, do not propagate it externally.

For additional help on writing good commit messages, see this article.

Comment Tags

We use the comment tags FIXME and TODO.

A TODO is a developer note-to-self. TODOs MUST be fixed before merging.

A FIXME is for things that should be fixed in the future. If FIXMEs cannot be addressed before merging, they can be merged, but they MUST include your username and a link to the issue. For example:

// FIXME(username): This is currently a hack to work around known issue X.
// Issue <LINK TO GITHUB ISSUE> will remove need for this work around.