Archive for April, 2012

Pull Requests

Friday, April 6th, 2012

What is a pull request?

A pull request is an announcement to others on a coding project telling them you have features you’d like to be merged in.

Why use it?

If you’ve got committing priveleges on a repo, you may ask why you shouldn’t just go ahead and merge your changes in.  Here’s why.

  • You won’t catch all of your bugs.  Having somebody else look over your code helps to ensure higher quality code gets merged in.
  • It keeps everyone involved up to date on new features being added, and gives them a say as to what gets added to the codebase.
  • It fosters conversation around the code.  Communication in coding, as in all aspects of life, is essential to success.  Pull requests mean you and your cohorts can hold each other to a higher standard.  This comes with the caveat that you’ll need to check your ego at the door–if you’re co-committers are any good then they’ll find something in your code that can be improved… every… single… time.

When to make a pull request

Always.  Even if you’re making a spelling correction in a comment.  To reiterate, it’s about communication.  And you just never know when something you do will be a mistake, or perhaps strike up a larger conversation.  Not to mention we all like reviewing an easy pull request, so it doesn’t hurt to throw out a gimme now and then.

If there are even two people working on a project then I’d advocate for using pull requests.  Unless, of course, there are only two of you and pair programmed the feature together.

The workflow

Now that I’ve got you thoroughly convinced of the benefits of pull requests, let’s get to the process.  First step is to get yourself a feature branch.

  $ git checkout -b my_feature

If you have already made commits towards your feature at this point then you’ll want to reset master to its original state.  First stash any loose changes you’ve got, then hard reset your master branch.

  $ git add .
  $ git stash
  $ git checkout master
  $ git checkout -b my_feature
  $ git stash pop

Now you’re set to start or continue building your feature.  Once you’re done with your features it’s time to compare your changes to master.

  $ git diff master

Go through your changes thoroughly, make adjustments, refactor, clean up.  Code review by others is for catching things you missed, not things you were in too much of a hurry to worry about.  I often end up going through my code changes three or four times, refactoring and making adjustments.

Once you feel comfortable with your code, go ahead and commit your changes.  Then push it to remote.

  $ git push origin my_feature

Now, if you’re using github you can navigate to the project page and you’ll see a button near the top of the page to create a pull request:
Github pull request button
Click the button and it will bring you to a page where you can see what your pull request is going to look like.  Click on the ‘Files changed’ tab and have another look over your changes.  On the ‘Preview discussion’ tab you can add some more details about your feature.  When you’re all set click the ‘Send pull request’ button and wait for the comments to come in.  Submitting the pull notifies all of the watchers on a project.

You’ll almost certainly get some comments on your pull and feel the need to make changes.  All you have to do is go back to your feature branch, make your changes, then push them up to your remote.  Github will automatically update the pull request with your changes.  It’s good to simply comment ‘updated’ so any followers are notified.

Once somebody has looked over your pull and approved you can go ahead and merge it in with the ‘merge’ button on Github.  Occassionally you’ll run into merge conflicts.  The ‘merge’ bar on github will be greyed out.  You’re going to have to first merge in master on your local machine.

  $ git checkout master
  $ git pull origin master
  $ git checkout my_feature
  $ git merge master

You’ll probably see some merge conflicts.  Open up the files listed as conflicting and fix them.  Afterwards resume the merge.

  $ git add .
  $ git commit
  $ git push origin my_feature

The merge bar should be green again on github.

Some tips on pull requests

  • Keep changes topical and atomic.  If you’re making a lot of changes, see if you can’t break them down onto separate feature branches.  The less code is in a pull request the faster it will get reviewed and merged.
  • Be anal.  Seriously, when it comes to code, you cannot be too OCD about the quality of your code.  When I’m reviewing your code, aside from simply verifying the functionality and structure, I’m going to be watching for trailing spaces, long lines and clean hash wrapping syntax.  Here’s a great style guide you can refer to.
  • Try to keep the commits relevant.  If you make a lot of commits while you’re working on your feature, you may want to do a git reset master once you’re ready to merge it in.  Then you can segment your commits a little more logically.

Reviewing pulls

Odds are you’ll find yourself in a position to be reviewing pull requests, too.  First off, you’ll want to download the code.

  $ git fetch origin
  $ git checkout their_branch

You’ll want to both test out the feature to make sure it works as expected, as well as look over the code thoroughly to make sure it’s up to standards.  Some things to watch out for:

  • Edge cases:  find the situations that aren’t being accounted for in their code.
  • Bugs:  code that doesn’t work the way it’s intended to.
  • Thorough tests:  testing should cover all of the edge cases and everything in between, and, naturally, they should all be passing.
  • Code quality:  check that code is clean, concise, and well organized.  Make sure the logic is handled in the best place possible and variables are named appropriately.  In Ruby the code should be largely self-documenting, but keep your eye out for situations where additional comments would come in handy.

Take it on as a challenge to find ways to improve their code.  It’s rare that a pull couldn’t use just a little bit more polish.

Once you are done reviewing the code, make a comment to show your approval.  I usually just say ‘lgtm’ for ‘looks good to me’.  Let the committer do the honors and merge it in.