An excellent discussion popped up on the Ministry of Testing Slack recently about pull requests. The person who started the conversation wanted to know.
What is the purpose? What are best practices? Does anyone have some resources to share? Our software department is new to git (moved this year away from Perforce for source control) and our department has varying levels and opinions about pull requests. Needless to say, there’s been some friction surrounding pull requests in the department.
Specifically around
adding suggestions and comments to the pull request. Some very vocal developers are of the opinion that the only comments allowed in pull requests are “blocker” comments like “if you check this in, it will break x”. Suggestion level comments are not welcome in the PR and are detrimental because they slow down the process. Others, including the majority of the test engineering group, are of the opinion that you can add suggestion-level comments as much as you want, just be sure to “approve” the PR if you don’t find any blocker-level issues.
So:
What, in your experience, is the purpose of a pull request?
What are some good practices that you’ve seen for them?
I feel that people who are arguing that suggestion level comments are not welcome are missing the biggest point of PRs - learning from well meaning reviewers.
It takes an incredible amount of effort to to do a good code review - sometimes even more than what the author has invested - because the reviewer not only has to read and understand the code but also think about how it could be better. A good code review from an experienced programmer leverages years of experience and can lead to a much more elegant implementation even if the existing implementation is correct. They might for example propose an O(1) alternative to your O(n) implementation because they know of a better algorithm or they might help you structure your class hierarchies and interfaces in a way that makes them easier for other programmers to understand and extend.
Besides insightful comments that reveal a deep understanding of your code structure and design there are also style comments from well-meaning folks just trying to add value. They might suggest things like fix indentation or changing variable names. This feedback may not be valuable for the code but how you react to it is extremely important for the culture you set for your team. If you push back and reject the feedback, you take away their ownership of that bit of code. Responding positively to such feedback helps them get invested and over time you can inspire them to give better feedback.
Remember all feedback in code reviews is a gift - perhaps limited by the reviewer’s ability to give. But good engineering culture teaches us to receive it gracefully nevertheless.
What, in your experience, is the purpose of a pull request?
Pull request serves if mulitple people contributing the source code in git, it helps to pull the latest code made by others.
What are some good practices that you’ve seen for them?
Here are the good Practices, which we use to avoid issues later:
->Always do pull request before committing any changes. So that if any issues it can be fixed before committing
->Using git ammend, which helps not only to save from mistakes on last commit but also provide better git history for easy tracking.
I use comments to give full feedback on PR from the user perspective. Then it is up to our team (which part I am) whether fix it in the current PR or in separate. This feedback helps to catch the issue on very early stages; also it reflects the user point of view that gives developer a better understanding of the whole picture from different angles.
Feedback on PR supports one of Agile principals: get feedback as soon as possible
The purpose of a pull request is for a few reasons
Sanity check. Engineers are human and can make mistakes, and while having another human review the code doesn’t solve for this problem, it certainly gives us some layer of protection to have another brain, another perspective, or differing experience look at the solution presented to determine if it’s going to work well, is scalable, presents a bug, or doesn’t actually solve the problem.
Automating your automated tests. CI is a big deal, and hooking this into your gitflow makes life so much easier. We’re in a process now where we’re working to completely prevent deploys to production if any of our tests are failing.
Education. Similar to the first item, I value other’s feedback on my work to the fullest. It is always great to hear of other potential ways to solve a problem, and if we don’t encourage feedback from each other we’re missing out on a great opportunity to learn from our peers.
It is good practice to be respectful of other’s time. Depending on the PR, time spent on it, or sheer size, it can be quite an overload to review, or to go back and forth on large architectural level changes. Here are some things we do to combat this:
Deliver code often. Rather than shipping a giant feature all in one go, consider how that feature can be broken up into smaller chunks that provide value. This is easier on cognitive load, faster in QA, and helps the project become successful earlier.
When a feature can’t be broken up, get feedback on the engineering plan, architecture design, requirements, etc. Finding core problems earlier on is way easier to correct rather than patching it later.
We comment everything we see. From styling issues, to typos, to bugs, etc. Does it sometimes feel like it’s not worth it? Yes. Is the state of the code better for it? Yes.
We also always specify whether each comment is blocking, non blocking but should be fixed later, or is non blocking and totally optional.