Skip to content

Reviewing Pull Requests

Jump to bottom Edit New page
Tom Schenk Jr edited this page Jun 26, 2017 · 1 revision

Requestor

Person who opened the pull request. The person may be part of the public or with the development team and are responsible for communicating with the Assignee(s) and Reviewer, updating the pull request, and following the contributor guidelines.

Assignee(s)

Responsible for shepherding the pull request and will have the ultimate responsibility to either merge the pull request or to close it without merging. The Assignee may also be the same person who opened the pull request.

The assignee should:

  1. Conduct a cursory check of the pull request and has followed the instructions in CONTRIBUTING.md, in particular, the DESCRIPTION file has been updated and that the Requestor has signed the CLA. Communicate with the Requestor if there are any issues.
  2. Assign the Reviewer to provide a technical review of the code.
  3. Accept the pull request only when there is consensus from the Reviewer.

Reviewer

The reviewer is responsible for double-checking the quality of the code. The reviewer must be on the RSocrata Core Development Team and should not be the Requestor. Reviews must be completed before accepting a pull request.

  1. Ensure the pull request has followed the contributor guidelines.
  2. Ensure the pull request is against the proper branch. Pull requests should be against the dev branch, unless it's a specific hotfix. This should have been made clear by the Requestor.
  3. Check the syntax, ensuring it is consistent with existing code. If not, comment on the code and point-out inconsistencies.
  4. Ensure that all tests have passed. If the write.socrata unit test failed, re-build tests to ensure that all tests pass on Travis-CI and AppVeyor. Note: code may not necessarily pass on local machines, reviewers should rely on the CI engines.
  5. Ensure new unit tests have been written for any new functions, (optional) parameters, or functionality. Check that the unit test is written concisely, clearly, and is grouped next to similar unit tests.
  6. Check code-coverage and ensure it has not significantly fallen, and if it has, ask for justification from the Requestor.

When finished, either approve or request changes. Comment liberally during the review. However, closing or merging the pull request should be left to the Assignee (the Requestor may also withdraw and close the pull request).

Clone this wiki locally