Code review tips

How do I get my code reviewed?

For Mercurial, you can use hg --rr <reviewer> push. This will initiate a post-commit review. For git, we just use the github process for right now. See the code review page for more details.

You can also use the kiln website to ask for reviews via the web. Go to the repository at khanacademy.kilnhg.com, hover over the changeset you want reviewed, and click "Review." Then select all the other changesets you want to include and the reviewers you want to see your code. They'll get an email asking them to come look at the review, and you can also just send them to the right URL.

How do I add new commits to an existing review?

(This is for mercurial/kiln.) Add the text "Review: <review #>" to your commit message. Or you can do it via the kilnhg website.  Be sure to push with no reviewers listed ("hg push --rr none"), otherwise a second review will also be opened.

How do I review someone else's code?

If someone asks you to do a review, you'll get email telling you what to do. If you want to review someone else's code without them asking, the easiest way is to go the khanacademy.kilnhg.com website, hover over the changeset you want to review, and click "review". Just assign the review to yourself.

What do I do if I want someone to review my code but it's not ready to be pushed to the stable deployment repository?

Easy. Just make a branch of the repository using the web interface at khanacademy.kilnhg.com and push your changes to the branch repository. (A similar technique works on github.) You can do the review from there without worrying about messing up a deploy. DVCS at its finest.

For any non-trivial commits, it's recommended that you push to your own branch repository and have your changes reviewed before pushing to the stable repo.

What is a recommended procedure for pushing?

Here, we assume stable and default are configured in your .hg/hgrc file:
default = https://khanacademy.kilnhg.com/Code/Website/Group/<yourbranchname>
stable = https://khanacademy.kilnhg.com/Code/Website/Group/stable
  1. Work in your local personal branch, without any regard for what's going on in the remote stable branch.
  2. Commit: hg com -m "fixed x"
  3. Continue working and committing as you make changes.
  4. Push your changes to your remote personal branch and specify reviewers: hg push default --rr <reviewer firstname>
  5. When you're ready, pull in changes from stable: hg pull stable
  6. Merge the changes: hg merge
  7. Commit the merge: hg com -m "merge"
  8. If the merge requires manual intervention, you should specify a code reviewer: hg push default --rr <reviewer firstname>
  9. Otherwise, no review is really needed for this trivial merge: hg push default --rr none
Basically, you want to make sure you push your changes for review BEFORE you pull from stable, or else you'll bundle all of the random changesets you pulled in from stable in your request for code review. Yuck. Review 1480 is an example of this gone wrong (before I, Stephanie, was enlightened):

Faulty push

Comments