How I Code Review

Code review is a healthy part of the development process. Unfortunately, the task can range from tedious to daunting depending on the size and content of the code change being reviewed. I just wanted to expose a couple of little tidbits that help me make the process as seamless as possible.

At Greenhouse, we host our code on Github and use Pull Requests to manage code changes.

A pull request on Github

Github provides a nice UI to look over changes in a Pull Request, leave code review comments, and eventually provide an approval for the pull request to be merged.

When reviewing anything non-trivial, I like to checkout that code and run it locally.

$ git checkout support-ruby
$ git diff master

I use RubyMine for Ruby development, and I like to use the “Local Changes” (cmd+9 to access the Version Control window) to easily navigate around a changeset. In order to get this going, we’ll need a few handy git commands.

$ git reset master
$ git status
On branch support-ruby
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)
 modified:   lib/figleaf.rb
 modified:   lib/figleaf/settings.rb
 modified:   spec/figleaf/settings_spec.rb
 modified:   spec/spec_helper.rb
Untracked files:
  (use "git add <file>..." to include in what will be committed)
 lib/figleaf/config.rb
 spec/figleaf/config_spec.rb
 spec/fixtures/errors/bad_file.rb
 spec/fixtures/extra/code.rb
no changes added to commit (use "git add" and/or "git commit -a")
$ git add .
$ git status
On branch support-ruby
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)
 modified:   lib/figleaf.rb
 new file:   lib/figleaf/config.rb
 modified:   lib/figleaf/settings.rb
 new file:   spec/figleaf/config_spec.rb
 modified:   spec/figleaf/settings_spec.rb
 new file:   spec/fixtures/errors/bad_file.rb
 new file:   spec/fixtures/extra/code.rb
 modified:   spec/spec_helper.rb

Now it’s really easy to navigate around the changeset!

ctrl+k brings up the “Commit Changes” window.

What’s nice here is that you can make changes to the code—perhaps code change suggestions—and when you’re done, git reset support-ruby will leave those changes intact.

$ git reset support-ruby
$ git diff
diff --git a/lib/figleaf/settings.rb b/lib/figleaf/settings.rb
index 67d8b50..3d79219 100644
--- a/lib/figleaf/settings.rb
+++ b/lib/figleaf/settings.rb
@@ -17,6 +17,10 @@ module Figleaf
         end
       end
+      def foobar
+        puts "YAY"
+      end
+
       # Public - configure auto defined settings attributes
       # and load yaml settings from confing/settings directory
       #

Now, if I’d like to share those code change suggestions, I can create a new branch and push that to Github:

$ git checkout -b support-ruby-with-foobar
$ git commit -am 'We need some foobar up in here'
$ git push -u origin support-ruby-with-foobar

Finally, I can open a PR of my support-ruby-with-foobar branch off of the original support-ruby branch.

Nothing mind-blowing, but I’ve mentioned this workflow to a few people who have found it helpful!