Code reviews are an integral part of open source software development, and software development in general. One of the greatest things about open source software is community involvement. A review an opportunity for someone with different ideas and knowledge than you to take a look at your code, and either verify it's good to go, or identify places where it can be improved. This applies to beginners and experts alike; everyone can benefit from a second set of eyes on their code.
Reviews should be a positive experience, regardless of the outcome. All feedback should be constructive and positive. Keep in mind, all feedback provided is regarding your code, not you as a contributor. Everyone involved needs to be receptive to feedback and willing to participate in the conversation.
Remember to be patient; sometimes it can take a bit for someone to review your code. We regularly check on open PRs and often request a review from our CircuitPython Librarians review team. However, a review request does not guarantee an immediate review. Many of the folks on this team are members of the community volunteering their time.
I've waited a bit, and I received an email saying there was an update to my PR. Time to take a look!
Below the commit list, there's a new section, beginning with "dhalbert requested changes 1 minute ago".
Dan, @dhalbert on GitHub, has reviewed my PR! Thanks, Dan!
Remember, you can always check the status of the PR to see where things are. The status of this PR is now Changes requested.
Some PRs are ready to go on the first try, and will be merged immediately. If this happens, excellent! You can skip the next section to find out how to continue.
Others require a bit of work first, as with my PR. The next section will take a look at what happens when changes are requested.
The Change Request
Many pull requests are not accepted in the state that they are initially submitted, especially as they get more complex. When a reviewer finds something that needs fixing or could be improved, they will submit a change request. This is, as it sounds, a request for you to make changes to your contribution.
GitHub makes this process seamless by allowing a reviewer to go through your code line by line and make suggestions exactly where the change needs to be made. This means you don't have to hunt through your code to figure out what the reviewer was referring to. It's important to read through the entire thing do you don't miss any of it.
The change request can be viewed in a couple of different ways. The first will show up as a series of comments within the Conversation tab of your PR. The second shows the comments in your code under the Files Changed tab. There's no right or wrong way to view your change request; you should use the method that works best for you. Let's take a look!
The Conversation View
The Conversation tab is often where you start when viewing your PR. A change request will show up in this tab as a series of nested comments, including code snippets. There are a few of sections within the request: the main review comment, the nested review comments, and reply or resolve.
Main Review Comment
This is the main review comment. Sometimes this contains only a general comment, and the important bits are below it. In this case, though, Dan has mentioned that there are two example files that I also need to update. This is what makes reading this comment so important! The GitHub interface does not allow you to comment on files not included in a pull requests, and therefore, if there are other files that need updating, the best you can do is mention them in a comment. Always read through what your reviewer has to say.
Nested Review Comments
Below the main comment, you'll find a series of nested comments that include code snippets from your pull request. They show the original code and your changes, color coded as in the other comparisons you've seen, and the associated review comments.
Dan's first suggested code change is a more precise name, and he has explained why the suggested name would be a better option.
He also included the Suggested change using the GitHub interface, so I can clearly see exactly what he means. Not all reviews will include this.
As there are many references to this new name, Dan also included a note about updating all of the references to it in the code.
Reply or Resolve
At the bottom of each nested comment, you'll find the option to Reply..., or Resolve conversation.
The Resolve conversation button is there for you and the reviewer. We use it in two ways. One is for the PR author to click when the update has been applied and pushed to the PR, and the author is satisfied that the requirements have been met. Two is for the reviewer to click when they are feel that their requested changes have been made, if the author has opted not to click it. Don't spend a lot of time worrying about whether you've done everything right before clicking; GitHub makes it simple to "unresolve" a conversation in the event that more is needed! Note that the usage of this button may differ based on the specific project to which you are contributing.
There will come a time when you receive a suggestion in a review that doesn't make sense or you don't agree with. You have every right to ask questions or discuss any part of a review. You can reply by clicking in the Reply... dialogue and typing your response. Pull requests are setup to handle forum-like discussions. Feel free to ask for clarification, explain the reason you chose to do something, simply thank someone for their assistance, or open any form of discussion you feel is needed for your review. Some more involved PRs have extremely lengthy discussions as code goes through multiple iterations and changes. This is great! You should always feel comfortable continuing the discussion if you feel it's necessary. We definitely do!
I decided to let Dan know that I agreed with his suggestion and will be making the change. My reply was sent to this specific change, so it is nested along with it. If Dan replied, the thread would continue as seen here. That way, if there are questions about multiple changes, they don't get crossed!
The Files Changed View
Requested changes in the Files changed tab show up in a very similar way, however, here they are shown within the updated file. This view is sometimes easier for folks because it provides a little more code context to the requested changes, than simply viewing the code snippets alone.
You can get there by clicking the Files changed tab or by clicking View requested changes at the top of the review in the Conversation tab.
Here you'll find the same review comment structure. Instead of only including the code snippet, it is nested within the overall file comparison. This can sometimes make it more obvious where in your code the change is being requested.
I agree with Dan's suggestions, so I'm going to go ahead and make the requested changes.
Submitting the Requested Changes
Submitting fixes for a change request can be done in two ways: using Git, and using the GitHub interface. However, there are many situations where the GitHub interface is not an option. This section covers each one, and explains the benefits of and caveats involved with both.
Using Git to Commit Suggested Changes
The process for using Git to commit suggested changes to a pull request is almost entirely identical to the process used to handle resolving the failed checks when the PR was created. One of the benefits to this is that you're already using a Git/GitHub workflow, so it may make sense to simply continue along that path. It also means that your local copy remains up to date with the remote copy on GitHub. Further, understanding how to use Git to submit changes to a PR is important; if you are asked to make changes to files not already included in the PR, you will need to use Git. So, it's convenient that the process is a familiar one.
Remember, pushing a commit to the current branch will update the open PR. This is valid for the entire duration of an open PR, regardless of the reason for submitting updates. Therefore, following the process you've already learned will get you the results you're looking for.
There were a few elements to the change request, including a rename, and updating two example files. As those files were not included in the initial PR, they'll need to be submitted using Git. I've renamed the argument/attribute io_object to io_mqtt in the adafruit_dash_display.py file. I have run my series of git commands: status, add, status, commit, status, push.
Excellent! This commit will now show up in the pull request. But, I have more changes to make. So, I'll make the changes to the example files, and then run the exact same series of git commands to commit my second set of changes to the PR.
Now it's time to check on my latest commit and see where the PR is at by looking at the status. There is a yellow dot next to the commit hash in the commit list indicating checks are occurring. The overall status is still Changes requested, because the changes have not yet been reviewed. As you can see, the checks are, once again, in progress.
You'll want to give the checks a chance to complete. If they fail, follow the process outlined on the previous page in this guide to resolve the issue. In this case, the checks have passed.
You can now scroll up to the first requested change, and you'll see that it has been marked as Outdated. This indicates that a commit has occurred that altered the specific line (or lines) of code addressed in that specific review comment. This does not mean that you've completed the change to the satisfaction of the reviewer, it simply means GitHub sees a change in that spot, and is noting that the existing review no longer applies.
There is one more change to address. This one we'll do using the GitHub interface.
Using GitHub to Commit Suggested Changes
GitHub includes a feature that allows you to accept suggested changes directly from a review comment, and commit them directly. This is a super convenient way to ensure that you're submitting the change exactly as requested by the reviewer. However, it is only available when the reviewer has provided a suggested change within a review comment. This means it is not possible to use this process to submit changes to files not already included in your PR. Further, it will result in your local copy of the code no longer being up to date with the remote copy. It's one simple step to keep up with this, but if you forget and push more changes, it can get hairy. You can use both Git and GitHub on the same PR. For my PR, I used Git to fix the naming suggestions. I'll use the GitHub interface for the documentation suggestions.
To begin, navigate to the specific change you want to commit. You can do this through either the Conversation or Files changed views. Click Commit suggestion.
The resulting dialogue comes with a prepopulated commit message, which will always be "Update filename.py", where the name and extension of the file match the file you are updating. It also provides the option to type in a commit description. You should continue your habit of descriptive commit messages and update the commit message. Once you've done so, click Commit changes.
The review comment will be marked as Outdated, and collapsed as completely resolved, because this method guarantees that the update matches exactly what the reviewer was looking for. If you need to see it again for some reason, you can always click "Show resolved" on the right. The new commit shows up in the list, the same as the others.
This is the last of the requested changes! The pull request is ready for a second look. You have a couple of options here. You can leave a comment tagging the person who began the review by including their GitHub username beginning with @, e.g. @dhalbert. You can also simply request another review by clicking the circle-arrows next to the reviewer's name in the Reviewers list at the top of the right column of your PR.
I've let Dan know that I made the changes, and now I'll wait until he's next around to see whether there's anything else to do for this PR.
Don't Let Your Local Code Get Behind
When there are more commits on a copy of code, whether local or remote, you call that copy ahead. The copy that does not have those commits is considered behind. At this point, the remote copy of the code is one commit ahead of your local copy, because you used GitHub to commit directly to the remote copy. When you are working on an open pull request, it's very important to stay on top of keeping your local copy up to date.
When you end up in this situation, you'll want to complete the following step. Return to your terminal program and run git pull remotename current-branch-name, replacing remotename with the name of your remote (your GitHub ID), and current-branch-name with the name of the branch you're using with the PR.
The local copy of the PR branch is now up to date with the remote copy. That way, if any further changes are requested that require using Git, it's ready to go.
Changes Approved
I received an email letting me know there was an update to my pull request. Dan's back, and he's taken another look. He is happy with the changes I made for the first two request comments, and has resolved both of those as well.
My PR has been approved! Dan has reviewed the changes I made following his change request and concluded that I've made them to his satisfaction. He comments to let me know this is the case, and approves the changes.
Merge
The final step in this process is having your pull request merged. A merge takes the changes submitted in the PR, and integrates them into the original copy of the code. Only the final commit is merged, as the final commit already includes all of the changes made in the previous commits.
Dan merged my contribution! My updates are now part of the Adafruit version of the Dash Display library, and moving forward, others may use and enjoy them.
Congratulations! You've successfully had a PR reviewed and merged. Now you're ready to pick out another issue to address, and continue your journey through open source software contributing!
Text editor powered by tinymce.
