A pull request (PR) is a feature provided by version control systems, such as Git, that enables developers to propose changes to a codebase. It serves as a mechanism for collaboration and code review within a development team or an open-source project.
When working with a version control system, such as Git, developers typically create a separate branch to work on a specific feature or bug fix. Once the changes are complete, the developer creates a pull request to merge their branch into the main codebase (usually the master or main branch).
The pull request contains a set of changes or commits that the developer wants to add to the main codebase. It includes details such as the branch being merged, the proposed changes, and a description of the purpose and scope of the changes.
Once a pull request is created, it can undergo a code review process. Other developers on the team or project can review the proposed changes, leave comments, suggest improvements, and discuss any concerns or questions they may have. The code review process helps ensure the quality and correctness of the code before it is merged.
After the code review, the pull request can be either approved and merged into the main branch, or further changes may be requested. Pull requests provide a transparent and collaborative workflow, enabling multiple team members to contribute to the codebase and maintain a high standard of code quality.
PR Process
Dev completes work on a Jira ticket and assigns it to a reviewer in JIRA
Dev creates Pull Request for the reviewer in GitHub following best practices below
Reviewer pulls the branch locally, tests per the ticket guidelines, and verifies it works as expected
Reviewer adds notes to the ticket on how to QA it (links to relevant pages on dev where the change appears, how to check to see if it’s working, etc)
Reviewer assigns ticket to PM for QA review
PM reviews the outcome to ensure it accomplishes the client’s goals
If approved, PM contacts the client for approval or updates
If not approved, PM documents the issue(s) and reassigns to the reviewer
Once client approval is received, code is merged to the main branch and deployed
Responsibilities
The Author: The author of the PR has the responsibility and room to make sure the PR is clear and approachable to other team members so that they can give a confident review.
Provide context to what brought about the Pull Request
Provide a summary of the problem the Pull Request is solving
Provide a summary of the solution that the Pull Request provides
The Reviewer: The reviewer of a PR has the responsibility to unblock the work of the author. They do this by approving the pull request, or by requesting needed changes and providing explicit feedback when necessary. No matter which way the review goes, it’s crucial that the review is timely so that they don’t block and frustrate their team members.
Unblocking the PR author
Validate the code: Make sure that the code in the PR looks like it is doing what the author intended it to do. This includes ensuring that there is proper testing.
Provide good feedback, which should include the following:
Clear, actionable advice with examples of what (if anything) should be changed.
Reasons for why the changes have been suggested, with documentation or evidence that supports that point of view.
An impersonal tone, so the review does not feel like overt criticism.
Do all this in a reasonable amount of time
The Team Itself: Good PR practices are helpful at an individual level, but to be most effective, the team should agree on and adopt a set of standards that the team can collectively hold PRs to.
Decide a reasonable time table
Decide what should be blocking vs non-blocking
Blocking
Bugs: This could be “This code will error when X happens. X happens often”
Untested code changes: How do we know if the code works if we can’t verify it?
Hard-to-Comprehend code: When a section of code is hard to read and hard to understand what it’s doing, it adds uncertainty to the code as well as a maintenance burden if it ever needs changes. We decided that’s a good enough reason to block code.
Non-blocking
Code styles: This is stuff like “could use .map here”. I call these “code flavors” because there is more than one way to do things and everyone has their own preference on what’s more “readable”.
Improvements: - This is stuff like, “What if we extracted this out to its own method/class?”
Opinions as Standards - Some people hold certain coding authors and their rules to a high standard. I love and take inspiration from some book authors, but I think that it’s important to see their recommendations as best practice opinions rather than rules to follow. Because of that, I find comments to follow those patterns as code styles and improvements and therefore non-blocking.
Best Practices
Respect People's Time
Always Provide Constructive Feedback
Keep Your Ego Out of Code Reviews
Reasons not feelings
Be open to other ways of doing things
Be Precise About What Needs to be Improved
Don't Just Hope for the Code to Work
Reinforce Code Submission Best Practices
Be Strict About Temporary Code
Check the Project’s Satellite Files (documentation, etc)
Visualize the Bigger Picture
Title the Pull Requests clearly, linking to the original JIRA issue if possible
For example: PROJ-123: Description
Provide a clear description of what is accomplished with the code change in the Pull Request, and link to the original JIRA issue when possible. For example:
This change turns the button color GREEN, because RED wasn’t very user friendly.### References- PROJ-123 - Getting issue details... STATUS
For small 1-5 line inline changes use GitHub’s inline comments feature, along with inline suggestions. For example… The button size should be 5 instead of 3. ``` suggestion $buttonSize = 5; ```
To Consider
When is a PR appropriate
Who is responsible for resolving merge conflicts
Goal of knowledge sharing as much as “approval”
How to prioritize/review style guide issues
Be generous of spirit – the goal is to produce code that works for functional web sites.