The pull request is critical to development teams. They provide a mechanism to check in about a piece of code and where it fits within the greater context. They are crucial check points to provide feedback.
Whether you are planning to contribute to an Open Source project. Entering the development work force for the first time. Or you are a seasoned programmer wanting to improve process. Making the most out of pull requests will change your development process forever.
No One Person Owns The Code
For pull requests to be effective everyone within the team has to check their ego at the door. It is very easy for developers to become invested in their code. This is OK if they are open to feedback.
The pull request is an opportunity for others within the team to provide feedback. Why do they need to provide feedback?
- To verify the code meets the standards that the team has defined.
- To confirm the changes meet the technical requirements.
- To understand the changes for future maintenance. *To make sure the implementation is supportable and maintainable.
These are three quick reasons that are hard to disagree with. A pull request is an opportunity for the team to provide feedback. The team owns and maintains the code, not a single developer. If this condition is not met it destroys team cultures and the pull request process.
How To Be A Good Reviewee
You have checked your ego and want feedback from your team. You have busted your ass developing a feature and are ready to open a pull request. It is critical that you do your part as the reviewee to get the most out of the pull request process.
- Verify all tests associated with the code pass before opening a PR.
- Remove any unnecessary code or comments before opening a PR.
- Try to keep your PR small as smaller chunks of code are easier to review than massive change sets.
- Provide the reviewer as much context as possible. Use the PR description to explain what you did, why you did it, and why you believe it to be the right solution. Call out any assumptions you made and anything you were unsure about.
These guidelines improve the pull request process. They optimize for effective, concise, and clear feedback opportunities for the reviewer. The last bullet point is by far the most important. The reviewer is going to be coming into your pull request cold. They will have a mental context switch. Be cognizant of this and provide as much context as you can about your code.
How To Be A Good Reviewer
Equally as important as being a good reviewee is being a good reviewer. This is an important role to be in for any given development team. The reviewer is making sure the code of another developer meets team agreements. These agreements could be team agreements, technical requirements, or feature requests.
It is important to recognize that an individual is looking to the reviewer for feedback. If you find yourself being a reviewer on a pull request, here are somethings to keep in mind.
- Don’t be an asshole.
- Always verify all tests pass.
- Never blindly approve a pull request. Verify the changes and give it your full attention.
- Understand the context the code was written in.
- Ask questions if something is not clear.
A good pull request should give the reviewer confidence. If it doesn’t that is OK because this is the time to catch these things. If as a reviewer you are not confident in the PR then provide constructive feedback.
- Explain what is missing and how to make it better.
- Help the developer understand why these things are necessary.
- Remember don’t be an asshole and instead be helpful.
If they are a new developer then you may need to do more than explain the issues. It may be the case that you need to help them out with an example. Take the time to help the developer understand why this pull request is not ready. Often times reviewers don’t do this. It wastes time, frustrates developers, and introduces friction in the PR process.
Help others out.
Conclusion
A pull request represents a checkpoint for feedback. The pull request process will be different depending on the project you are working on. But the feedback process will always be present in some form or another.
There will always be a developer needing feedback from someone else. These guidelines are talking about pull requests, but they apply to general feedback. As someone that needs feedback explain where you are, how you got there, and things you are assuming or not sure about. Then as someone that is providing feedback, don’t be an asshole, give the problem your full attention, and help the other person.
Hungry To Learn Amazon Web Services?
There is a lot of people that are hungry to learn Amazon Web Services. Inspired by this fact I have created a course focused on learning Amazon Web Services by using it. Focusing on the problem of hosting, securing, and delivering static websites. You learn services like S3, API Gateway, CloudFront, Lambda, and WAF by building a solution to the problem.
There is a sea of information out there around AWS. It is easy to get lost and not make any progress in learning. By working through this problem we can cut through the information and speed up your learning. My goal with this book and video course is to share what I have learned with you.
Sound interesting? Check out the landing page to learn more and pick a package that works for you, here.
Top comments (5)
What's your opinion on opening pull requests for WIP? Does it work when your team is small? Does it not scale well if there are a bunch of pull requests already open?
In my experience, I don't typically do WIP pull requests. To me, branches/in-flight work should be kept very small so that a complete/non-breaking PR is opened every day ideally. That said, breaking work down into those small chunks is not trivial, nor is "non-breaking".
I believe opening PR's for WIP could be beneficial if ad-hoc code reviews with teammates is not an option. I would prefer one on one code reviews for that type of WIP work. The reason for that is because of scale, as you mention a larger team with multiple WIP PR's is likely going to become unmanageable.
I like your point about trying to keep PRs very small. Even though breaking work down into chunks may be difficult, I feel the time spent is worthwhile. It provides a nice way to further understand the work you'll be doing vs. starting to add commits wildly to the PR as you go along.
Would you say you practice trunk based development then? We don't follow it exactly as prescribed, but it's sooo much easier to reason about code when the changes are small.
Does it mean you have to pull the PR branch to test the code yourself?
It depends on your CI/CD pipeline. I tend to have one setup that will just run the suite of tests and check ins to your features branch triggers a build. Then a developer can verify the test pass during the PR. The reviewer should still pull the code down if it is more than a one line code change to validate things have been accounted for.