Skip to content

Latest commit

 

History

History
59 lines (42 loc) · 4.1 KB

README.md

File metadata and controls

59 lines (42 loc) · 4.1 KB

Code Review Guide

All code in Cesium is publicly peer reviewed. We review code to share knowledge, foster shared ownership, and improve code quality and consistency. Just knowing that code will be reviewed improves its quality.

This guide describes best practices for code reviews.

General

  • GitHub has great tools for code reviews in pull requests. Become familiar with them.
  • If we don't have a CLA for the contributor who opened the pull request (or, more precisely, any contributor to the branch), politely ask for one before reviewing the pull request (example).
  • Most pull requests require additional work, often minor but sometimes major, before being merged. It's not a big deal. Sometimes we open a pull request with a task list for early feedback.
  • Anyone is encouraged to review any pull request that interests them. However, someone familiar with the changed code should ultimately merge it.

Reviewing

  • See the forest through the trees. Don't just review code one line at a time. Consider the big picture and its implications.
  • Comments are about code, not the contributor who wrote the code. Don't be offended by a reviewer's comments and don't aim to offend when commenting. We all want the same thing: to improve Cesium.
  • Provide motivation when it isn't obvious. Suggest why a change should be made.
  • Point contributors to a relevant part of the Coding Guide when useful.
  • Be concise. Make every word tell.
  • Be responsive. The contributor should expect prompt feedback from reviewers, and reviewers should expect the same. If not, politely ask for it. We all want pull requests to get into master.
  • Limit the scope. As a reviewer, it is easy to want to increase the scope, e.g., "why don't we do this everywhere?". These are often fair questions but can be better served by submitting a separate issue to allow more incremental pull requests.
  • Bring others into the conversation sparingly. If someone has expertise with a particular language feature or problem domain under review, invite them to comment.
  • If an experienced contributor makes a occasional whitespace or trivial mistake, just fix it to save on noise and speedup the review.

Changes to the Public Cesium API

  • If new identifiers were added to the public Cesium API:
    • Verify there is new reference doc. See the Documentation Guide.
    • Verify that CHANGES.md was updated.
    • Does the change warrant a new Sandcastle example?
    • Does it warrant a blog post or tweet so users know what to look forward to in the next release?
  • Verify that deprecated and breaking changes to the public API were handled correctly. See the Deprecation Guide.

Testing

  • Don't just review the code; test it by running the unit tests and relevant Sandcastle examples. See the Testing Guide.
  • For some changes, it is useful to profile Cesium or step through the code in the debugger.
  • Read the new reference doc. Build the reference doc if the changes are significant.

Merging

  • Cesium uses Travis CI for continuous integration. Travis automatically builds Cesium, runs JSHint, and generates the documentation for each branch pushed to GitHub. Before merging a pull request, verify that all Travis checks pass, indicated by the green check-mark and green Merge pull request button:

  • Delete the branch after merging the pull request.
  • Verify that the corresponding issue (if any) was closed.

Resources