Pull Request Code Review Procedure

Sandstorm Code Reviews

This is a quick overview on how and why we review pull requests, especially relevant to core developers (the people who have write access to the repo and so could bypass pull requests if they wanted).

Tactics:

• Every piece of code is "owned" by someone. Ownership is tracked on the table below.
• Every pull request should be reviewed by the owner(s) of the affected code.
• When editing code you own, you do not need a review, but you may ask for one if you feel it may be helpful.
• The owner will click "merge" when the pull request is ready.
• Review comments should be addressed in a new commit without squashing previous commits, so that the reviewer can see what changed without re-reviewing everything.
• After addressing comments, comment on the PR to notify the reviewer that it is ready for re-review.

Strategy:

• The purpose of code reviews is to make sure that for any piece of code, someone is aware of everything happening in the code. Conversely, someone who is aware of everything happening in the code is in the best position to know if your changes are appropriate or may cause any problems.
• A secondary purpose of code review is to spread knowledge between engineers. The reviewer may point out ways that the code could be organized better, or point out trivial violations of the style guide, perhaps marking the comment "nitpick", "style", "tip", or "suggestion". These comments aren't necessarily important enough to hold up merging the PR, and the PR author should feel free to push back if such comments seem not worth fixing or if the reviewer is causing the review to drag on too long.

Ownership

• Sandstorm front-end implementation: Kenton
• Sandstorm back-end implementation: Kenton
• Sandstorm regression tests: Jason
• Sandstorm web site: Kenton
• Vagrantfile: Asheesh
• Installer script: Asheesh
• Sandcats: Asheesh
• Documentation (docs.sandstorm.io) and related scripts: Asheesh
• Apps ports: Each owned by the person listed as the port author in the app list.
• vagrant-spk: Asheesh or Drew

For anything not listed, either assume Kenton or ask Kenton to document.

Stylechecking

Sandstorm now has automated stylechecking. You can run the stylechecker locally with make stylecheck. Also, you can add a git pre-commit hook that will automatically do this check only on changed files with this script.