A pull request(PR) is a request for the organization to pull some of your work into the master codebase. This document codifies the rules for opening and merging a PR at RailsCo.
At RailsCo we take our craft very seriously so like any important piece of work, code undergoes an editorial phase before being merged. Pull requests are the main component of this editorial process, and they are the only acceptable way for code to enter the master codebase here at Rails Co.
Because the editorial process is expensive, both in terms of mental power and time taken, there are a few things that must be done before a Sr. Engineer will look at your code. For each new feature or bugfix you submit, run through this checklist first:
You must have evidence that you've completed the feature or fixed the bug in question. At RailsCo, this evidence should be in the form of tests.
Experiments do not belong in the master codebase. Every new feature should be accompanied with a suite of tests that describe the expected behavior of the new code. Every bugfix should come accompanied with a test that can recreate the bug, as well as the code that fixes the bug (and therefore the test).
If the existing test suite fails after your modifications, your code will not be accepted into master. No PR will be reviewed until the existing tests all pass.
Make sure any degugging code gets removed from your code before opening a PR. This includes things like binding.pry (in ruby) and debugger statments (in JS), as well as things like puts and console.log.
Any code not related to the feature being worked on should be moved to another branch before opening a PR. The PR should be very precise to the ticket you're working on.
Once you've got clean code that passes the tests, you'll open a Pull Request. This is roughly equivalent to submitting a rough draft for editorial review. A Sr. Engineer will be reviewing your submission for a variety of things related to both functionality and readability. Once they've run through the code, they will decide to merge the code, or tell you what needs to be fixed before the code can be merged.
In order to keep code review streamlined, there are a few things you must do with every PR you open.
Write a paragraph about the work you did. If this was a new feature, describe the feature. Tell your reviewer in the PR what the feature will be used for and desribe the tactics you used to implement the feature. Be detailed! Without understanding the context for your work, a reviewer will not be effective at determining if the code does what it's supposed to.
For a bugfix, describe how to reproduce the bug in the current master codebase. Describe why this behavior resulted in a bug. Describe how your changes fix this behavior.
If there is a pivotal tracker ticket for this feature or fix, put a link in your PR to the ticket.
If some sections of your code are crucial to the feature, or more complex, then call attention to them. Ask questions that you want answered, and raise your own concerns if you're unsure of something. Questions like "Can you think of a simpler way to do this?" or "Will changing the value of this global variable cause issues down the line?"
Remember, this is an editorial process -- but the code is yours. The most important goal of code review is to improve the quality of all code going into master.
Opening a PR is often not enough to get the attention of your teammates. Send a friendly email to the person who should review your code (typically the lead dev on your team) with a link to the PR. This should alert the Sr. Dev that you're ready to have your code reviewed.
The vast majority of PR's result in feedback, rather than being merged right away. This is the goal of a good PR after all. The feedback you recieve may be follow up questions (Why did you choose to use .map for this?), suggestions (I we should rename this variable), to non-optional changes (This code creates a security vulnerability, you cannot use eval() on code from a third party API call).
Because you are the owner of this feature / fix, some of the decisions are up to you. If you disagree with the reviewer on a few decisions, feel free to respond respectfully to the feedback. Here are the steps you should follow when responding to feedback.
If you don't understand the feedback, or need more details about something, ask questions on the PR. The editorial process is a negotiation and back-and-forth is to be expected. Make sure you feel confident that you understand the feedback before taking action. Most of the time this can be done on GitHub or Slack.
Once you understand the feedback, make the changes that the reviewer has asked for. Make the changes on the same branch, and push the new changes to the same branch that you opened the PR against. If you do this, GitHub will automatically pickup the changes in the PR view.
Notify the Sr. Dev assigned to your PR that you believe you've made the changes they want, and that you're ready for another round of review. They will look at the changes you've made and decide if anything further needs to be done before merging.
Once your code passes review, the Sr. Engineer will merge your code to master (Rather than asking you to do so). The Sr. Engineer will leave a message on the PR to the effect of "Looks good to me, I'll merge this code!" and you can close any tickets associated with the work you've just done.