Reviews are a crucial part of the contribution process. They can also be a choke point in the process, as often the number of people available to do reviews is significantly less than the number of contributors. Our solution is to accept reviews from a wider range of people. Even if you don't have write access to a repo, you're always welcome to provide a review on a pull request.
You may be saying to yourself, "But, I'm new to all of this, what do I have to offer as a reviewer?" Everyone has something to offer! Pull requests can consist of everything from simple typo fixes to massive core changes, and every single one of them has the potential for bugs. Bugs can be anything from the code not working at all to a typo in a docstring that the linter didn't catch. Even if you aren't an experienced programmer, you have what it takes to be a capable reviewer.
Don't worry about approving a PR you don't entirely understand. Simply be clear regarding what you checked. For example, if you checked it for typos, include that in the comment section. "Checked for typos. Looks good!" If you tested the code on your own board to make sure it works, let us know. "Verified this works on the correct sensor." This allows you to review the parts you do understand, while giving us the opportunity to check the part of the PR outside your scope.
Many fear the review process because they've had a negative experience receiving a review in the past. We strive to mitigate this by perpetuating a positive experience through positive, constructive feedback. We always thank people for their work before providing feedback. Much of the work contributed to open source projects is done so by members of the community. The most important things you can do is make sure those people feel valued as contributors, and help build their confidence. This is an essential part of how we operate and we expect anyone else who participates to do the same. Always consider how you would feel receiving the review you're about to give and make sure that it's absolutely positive about it.
Any feedback can be positive, constructive feedback. It's entirely in how you present it. Simply telling someone, "You're wrong," isn't positive or constructive. The same information can be presented by saying, "Thanks for doing this! I have a suggestion. The change on line 17 could be done differently," followed by your suggested change. Now you've started a conversation. You've provided a review that gives the person a place to start from for improving their changes, and helped create an environment where they can feel confident. This is the most important part of the review process.
This section walks through the steps of giving a review. In this review, I'll be requesting changes to the code and then verifying those changes were made before approving. Let's get started!
I've been paying attention to a few repos and I see that Sommersoft opened a PR. Excellent! That means it's time for a review.
The first thing you want to do is wait for Travis to finish testing. If Travis fails, then the person who opened the PR will need to submit the fixes for that. While you're welcome to begin the review with suggesting Travis fixes, it's usually a better idea to give the person a chance to take care of the issues first.
For a detailed look at the elements of a PR, check out the Pull Request Explored section of Open Pull Request.
In this PR, Travis builds successfully and all of the checks pass. Now I can start my review.
Reviewing begins with looking at the changes included in the PR. Click on the Files Changed tab under the title and status of the PR. This will take you to the diff of all of the submitted changes.
Sommersoft has added a
not to line 175 and 191. I see some problems, however. It seems like the code will only run if the values are outside what the error says are the appropriate values. It's an easy thing to miss. So, I'm going to start a review to let Sommersoft know my thoughts on the PR.
If the issue you find is something applicable to the entire piece of code, you can simply click the Review Changes button at the top left, and leave a comment there. However, when you find issues on specific lines, it's good to leave an inline review with comments at the specific points you're referring to. The issues I've found are on specific lines, so I'm going to begin my review with a line-specific comment.
Mouse over any part of the line of code and you'll see a blue plus appear next to the line number.
Click the blue plus and a window will appear below that line of code.
In the comment field, I'm going to write up my first review comment.
It's always possible that there's a reason the contributor chose to make the submitted changes. If you're unsure, ask about the change. A review doesn't have to be suggested changes, it can simply be a question about why a change was made.
I'm going to ask about the acceptable values. In this case, I'm certain that it's not right as it is, and I have an idea of how to fix it. So, I'm also going to suggest a change to the code. GitHub supports Markdown in comments, so I'm going to format the code so the comment is easier to read.
If you'd like to know more about Markdown, click the link in Styling with Markdown is supported below the comment field for details.
Once I'm done, I'm going to click the Start a review button below the comment field.
This begins my review by saving this first comment. It won't officially submit it until I'm ready. It will instead show my comment as Pending until I choose to complete the review.
Now I'm going to repeat the process for the second issue. First mouse over any part of the line to bring up the blue plus sign.
Click the blue plus to bring up the review comment window. Fill in your comment. In this case, since the issue is the same concept as the first, my comment is shorter because I can refer to the existing comment.
This time, since the review is already in progress, the button has changed. Click the Add review comment button to save the new comment.
Now my second comment is pending as well. These are all the comments I would like to make to specific lines of code, so it's time to finalise my review.
Notice that the Review changes button located above the code has a 2 next to it. This is because there are 2 pending review comments. Since I'm ready to finalise my review, I'm going to click the Review Changes button.
This will open window over the code with a comment field and some options below it. This is where you can add the main comment seen in the review. Whatever you put in this comment field will be at the top of your review, with your in-line comments below it. Here is where I'm going to thank Sommersoft for the changes he submitted, and let him know that I have a couple of suggestions. Since I outlined these suggestions in the pending comment, I'm not going to reiterate them here.
Notice that there are three options at below the comment box. It defaults to Comment which allows you to submit a review containing only a comment if you choose. Below that is Approve, which you will choose when you're ready to approve the PR. Last is Request changes, which is what I've done in my pending comments, so I'm going to choose it as my option.
Once I choose Request changes, I'm ready to submit my review. So, I click the Submit review button at the bottom of the window.
Once I click Submit review, it will automatically take me back to the Conversation tab and show me my review. Since I requested changes, the status at the bottom of the page now reflects that.
Now I'll be waiting for a response from Sommersoft.
I received an email letting me know that there's been a response from Sommersoft on the PR. I'll navigate to the PR again, or refresh the page if I already have it open.
Sommersoft has commented in response to my review comments. He responded to each one individually, so each response shows up below my in-line comments in my review.
In his first response, he's answered my question regarding the acceptable values, indicated I was correct with my suggestion, and let me know he will be submitting an update soon.
His second comment is similar to mine in that it simply answers the question asked. He's already let me know he will be submitting an update, so he hasn't reiterated it.
Great! I don't have anything further to add. Sommersoft understands the issues and will be fixing them, so now it's time for me to wait until he submits the fix.
Next, I receive another email letting me know that he's committed a change to the PR. Now I can return to the PR and view the updated changes.
Remember that a PR is visually a timeline. So, the newest commit will show up at the bottom of the list.
Click the View Changes button next to the most recent commit to return to the diff.
This will take you to the diff showing the most recent changes.
Note the blue information at the top of the diff stating Viewing a subset of changes. with a View all link. This is because I navigated to the diff using the View changes button next to the most recent commit. In this case, it is in fact all of the changes, however in some cases, the last commit may only affect a small part of the overall PR. This is handy if you requested changes on only a small part of the PR. It allows you to view only the changes you asked for instead of scrolling through a lengthy PR to find what you're looking for. If you wish to see the entire PR in that case, click View all and it will return the diff to the original view.
As you can see, Sommersoft took my suggestion for how to resolve the issue with the code. Excellent! I'm ready to approve the PR.
Click on Review Changes at the top right of the screen. It will open the same window as before with the comment field and options. This time, I'm going to choose Approve, and include a comment to go with it. When I'm ready, I'll click Submit review.
Once I click Submit review, it automatically returns to the conversation tab. My change request comments are now collapsed with the option to Show outdated. Below the final commit, my approval is listed as such with a green check next to my name.
For those without write access to the repo, this is the end of the review process. This completes a significant portion of the entire review and greatly helps those of us with write access. We really appreciate it!
You've reviewed, requested changes, verified and approved the pull request. For those with write access to the repo, the final step is merging the PR. After you submit your review, the PR automatically returns to the conversation tab. You'll see your approval comment, and there should be green checks throughout the status section.
The Merge pull request button should also be green. Click the Merge pull request button.
This replaces the entire status section with the confirmation window. There's typically no reason to change any of the populated messaged in this window.
Click the green Confirm merge button to continue.
Done! The status section at the bottom is immediately replaced with a purple dot followed by the fact that I merged the PR into the project's master branch. It can take a moment for the rest of the PR to catch up - the status at the top may show open for a bit after the merge is complete. Once it's set, the status at the top also showed Merged in purple, followed by outlining the number of commits I merged, and the branches merged to and from.
This PR is all set to go! Sommersoft can now do what he wants with his branch and continue on to his next project.
This PR went quickly. As is indicated by the timestamps, it took about an hour. Some PRs can take days or weeks depending on the level of changes or review involved. We look at PRs as a conversation. This means some are submitted early on in the process with the intention of receiving an extended review to help with development. You're welcome to join in on this conversation at any time to provide feedback. We value the contributions of our community members, regardless of what side of the PR they're coming from. Thank you again for being a part of this!