The problem with pull requests / code reviews

We use git and GitHub’s Pull Requests fairly heavily in developing Close.io sales software. My main problem with pull requests as code review is that it focuses my attention too much on the specific lines of code added/removed/modified, but not enough on the context of where they were added/removed.

Take this view, for instance:

Sure, these 3 lines look fine — there are valid Python code and don’t contain any obviously flawed logic. And I can see which file it’s in (app/resources.py), and even the method name (‘validate_request’). But this very often isn’t enough for me to really review what’s going on. Do these lines make sense here? I have no idea from this view.

One reason is that seeing 2 lines of code above a change doesn’t really give me enough context even within a method. Another reason is that it’s not uncommon (especially in Python apps) to have a bunch of similar classes in the same file, that may all implement the same method names. In this case, how do I know which Resource class this code was added to?

Now, GitHub is just showing “git diff” which includes 3 lines (including whitespace) of context by default. But between git-diff’s additional options like –unified/-U (which allow you to show more than 3 lines of context) and  –function-context/-W, as well as the ability for them to combine AJAX-ey goodness, there’s a lot of opportunity for improvement.

Here are some ideas:

  • No UI Approach: For starters, add a querystring feature like ?u=7, which would show me 7 lines of context instead of 3. They already do this with ?w=1 to show a diff that ignores whitespace changes and it comes in handy. This “no UI” approach is a no brainer, in my opinion.
  • Get fancy with JavaScript: See the little “…” above line 422 which represents all the extra code? This is the web… I should be able to click on the ellipses to expand the context. It could load in and display several extra lines of code when I click this to let me see additional context about the code I’m reviewing.
  • Get fancy with git: I’m not sure exactly how git-diff shows the function (def validate_request(self, obj=None): in this case) at the top of each chunk, but some intelligence could be added so that showed the class/object name in addition to the function/method names.

Can you think of a better way to solve this problem?

 

Follow @philfreo on Twitter

Want to know when I write another post?
(only about once per month)

7 Comments

  1. Mark V. said,

    May 1, 2013 @ 1:09 am

    Bitbucket implemented suggestion #2: you just expand diff context manually, some 10 lines per click.

  2. Phil Freo said,

    May 1, 2013 @ 1:11 am

    And so they did! Could be a bit faster and perhaps use some slick animation, but overall seems to work well.

  3. Craig said,

    May 1, 2013 @ 1:18 am

    Agreed! I highly recommend checking out the branch with a JetBrains IDE, then comparing it with master. This will show you a full diff and even allow you to make edits and push a revision.

  4. Terry A. Davis said,

    May 1, 2013 @ 1:24 am

    I take things out of context and make them fit the current context of conversation. Suit yourself. http://www.youtube.com/watch?v=eB5VXJXxnNU

  5. Greg Banks said,

    May 1, 2013 @ 1:31 am

    1. Install xxdiff or meld
    2. git fetch $their-repo
    3. git log –oneline $new-branch
    4. git difftool $commit

  6. Wilfred said,

    May 1, 2013 @ 9:09 am

    Git diff shows the function name by examining indentation and finding the last line before the diff that is indented less than lines in the diff. Anything smarter would require more language-specific smarts. It’s doable, of course, but the only generic tool today that I know of that does this is Emacs’ which-function-mode.

  7. Geek Reading May 1, 2013 | Regular Geek said,

    May 1, 2013 @ 12:00 pm

    […] The problem with pull requests / code reviews (Hacker News) […]

RSS feed for comments on this post