wiki:code-review

Version 10 (modified by jgross@mit.edu, 6 weeks ago) (diff)

Update information about comments about GitHub

BarnOwl has a pre-commit code review policy. Essentially all code that goes to the master branch of the primary repository should be reviewed and ACKed by at least one other developer.
  • There is no official standard for who is allowed to ACK what. It suffices that both the author and the reviewer feel confident in their ability to evaluate the change.

    Larger changes, however, such as significant refactorings or design of non-trivial features, should be informed by broader discussion whenever possible.

  • An important part of code review is explicit communication from the reviewer. Review comments should include either a clear ACK, or one or more clear action items for the developer (generally of the form "make this change" or "answer this question").

  • The use of a "Reviewed-by:" line by the reviewer is suggested but not mandatory as an indicator that the reviewer is happy with the patch in its present form. Developers are encouraged but not required to include any applicable "Reviewed-by:" lines in the commit message of the patch when it is merged.

  • While code review of small patches may happen over Zephyr, or larger patches if a convenient reviewer is present and paying attention, decisions or review of non-trivial branches should, in general, happen at least largely over email (to barnowl-dev@mit.edu). Pull requests made on GitHub (http://github.com/barnowl/barnowl/pulls) automatically send email to barnowl-dev@mit.edu, as do comments on the pull request itself. (Comments on individual commits do not result in email notification. Therefore, please make all comments on the pull request itself, or on lines of the overall diff.) Note that GitHub emails show up, at least some of the time, as being from notifications@github.com and to barnowl@noreply.github.com.

  • Review requests made by any medium other than GitHub pull requests should include at least one of:

    • The location of a git repository containing the changes.
    • The patch(es) themselves inline in email (using e.g. git format-patch and git send-email).

    In general, the latter is suggested only for short patches or patch series. It is preferred but not required that such requests also include the location of a git branch.

  • If a review request to the mailing list is not responded to in any way within seven days from time of sending, the developer may, at their option, merge it to master without review. As pull requests on GitHub now send email to the mailing list, this timeout policy also applies to GitHub pull requests made after May 20, 2013. There is no such timeout for review requests communicated in any other way.