In the fast-paced world of software development, the process of reviewing and approving Pull Requests (PRs) is a cornerstone of collaborative coding. It's where ideas meet scrutiny, where quality is honed, and where the rubber meets the road. Yet, there's a pervasive myth in many development circles that a PR is only worthy of approval if it's perfect. This notion, while well-intentioned, misses the mark on what Agile and iterative development truly stand for.
Improvement Over Perfection
At the heart of Agile development is the principle of continuous improvement. It's about making small, incremental changes that collectively enhance the product. This philosophy should extend to our approach to PRs. A PR should not be expected to achieve perfection but to make a positive step forward. If a PR enhances the user experience (UX) or improves the developer experience (DX), it deserves approval. Here's why:
Enhancing User Experience (UX)
Improvements in UX can range from minor tweaks that make the interface more intuitive, to major features that significantly enhance user satisfaction. Each step forward contributes to a better product. By waiting for a PR to be perfect, we risk delaying these improvements and, by extension, impacting the user's interaction with the product negatively.Improving Developer Experience (DX)
Developer Experience is just as crucial as User Experience. A PR that simplifies code, enhances readability, or reduces technical debt makes the development process more efficient and enjoyable. These improvements may not always be monumental, but their cumulative effect is significant. Approving PRs that contribute to better DX ensures that the development team remains agile, motivated, and less prone to burnout.
The Agile mindset is about embracing change, learning from feedback, and making continuous improvements. By applying this mindset to the PR review process, we encourage a culture of collaboration, learning, and adaptability. It's not about making every piece of code flawless but about moving forward together, improving with each step.
When to Approve a PR?
So, when is the right time to approve a PR? The answer is simple: when it improves the product or the process, even if it's just by a small margin. Here are some guidelines to consider:
Does it improve UX? If it makes the user's experience better in any way, it's a step in the right direction.
Does it enhance DX? If it makes the development process smoother, more efficient, or more enjoyable, it's worth approving.
When to Reject a Pull Request and how
While we champion the approval of PRs that drive improvement, there are circumstances where rejection is not just warranted, but necessary. It's crucial, however, to approach rejection with a constructive mindset, aiming to foster learning and growth rather than discourage contribution. Here are clear-cut scenarios where rejecting a PR should be considered:
Continuous Integration (CI) Failures
When the CI pipeline fails, and it's not due to flaky tests, this is a red flag. A red CI indicates that the code changes have introduced errors or have failed to meet the existing project standards and requirements. It's essential to ensure that all tests pass successfully before considering a PR for approval. This ensures that the new changes do not adversely affect the application's functionality or stability.Breaking the Primary Use Case
If a PR compromises the primary use case of the application, it necessitates a rejection. The core functionality represents the value proposition to your users; thus, any disruption to this could significantly harm the user experience. Before approval, it's imperative to verify that the main features of the application remain intact and functional.Critical Performance Regression
Performance is key to user satisfaction. A PR that introduces a measurable, critical performance regression detracts from the user experience and undermines the efficiency of the application. Such regressions should be identified through performance benchmarks or profiling. If a PR significantly worsens the application's performance, it should be reworked before reconsideration.
It's crucial to reject with a clear message about what needs to be changed/fixed. Conventional comments really help with that.
The Role of Testing
Implicit in these criteria is the assumption of a comprehensive testing strategy, encompassing everything from unit tests to end-to-end (E2E) tests. This robust battery of tests serves as the backbone of a reliable, resilient codebase. It enables teams to catch issues early and with confidence, ensuring that any changes introduced do not inadvertently harm the application.
Navigating Rejection Constructively
Rejection should not be the end of the road, but a pivot towards improvement. It offers an opportunity for learning and development. When rejecting a PR, provide clear, constructive feedback that helps the contributor understand the reasons and what they can do to address the issues. Encourage them to re-submit their PR once the concerns have been resolved, reinforcing the idea that the review process is a collaborative effort towards maintaining high standards, not a barrier to contribution.
Healthy rejection rate
Are we saying that you should approve all PRs? In perfect world yes.
We don't live in perfect world-you will have rejections. Just watch out so that it does not get out of hand. Rejection rate is IMHO a very good metric for how well your team is able to execute. If it is high something is wrong in your team.
Best produtct teams I have worked on have low rejection rate like 6-14 percent.
Anything over 40 percent is a problem.
Your team is misalligned on where the quality bar for your product is or you're missing important automated tests which could prevent from the PRs being opened in broken state in first place.
On the other hand lower than 5 could mean that your developers do not review proprely and just stamp whatever PR gets opened.
Even if you work in a perfect team, where everyone writes the best possible code, even in this scenario I would advise to do PRs to avoid knowledge silos and spread knowledge about the system.
Conclusion
The decision to approve or reject a PR should always be made with the product's and the team's best interests in mind. By setting clear criteria for both actions, teams can foster a culture of continuous improvement, collaboration, and high quality. Remember, every approved PR is a step forward in the journey of your product development. Reject only when absolutely necessary.
Top comments (0)