What We Can Learn from the Ruby on Rails Project about Code Review

What We Can Learn from the Ruby on Rails Project about Code Review


About the author

Will Barrett is a Software Engineer, Technical Lead, and Engineering Manager from the San Francisco Bay Area with over 14 years of experience. He’s a Superset PMC Member of The Apache Software Foundation. He’s held staff software engineer and senior engineer roles at Change.org, Entelo, Sqwiggle and Preset.

Will is the author of On Learning to Program, a blog for new Software Engineers entering the industry. Will is also a certified reviewer on PullRequest where he’s caught hundreds of bugs, security issues, and other critical issues for over 40 teams.


images/ruby-on-rails-github-pull-request-code-review.jpg

What is the Ruby on Rails Project?

Rails is a web-application framework that includes everything needed to create database-backed web applications according to the Model-View-Controller (MVC) pattern.

— From the Ruby on Rails README on GitHub

The above description understates the scope and capability of Rails. Built on the Ruby programming language, the project has created an extensive set of features spanning many packages that work together or separately:

Active Record, Active Model, Action Pack, and Action View can each be used independently outside Rails. In addition to that, Rails also comes with Action Mailer, a library to generate and send emails; Action Mailbox, a library to receive emails within a Rails application; Active Job, a framework for declaring jobs and making them run on a variety of queuing backends; Action Cable, a framework to integrate WebSockets with a Rails application; Active Storage, a library to attach cloud and local files to Rails applications; Action Text, a library to handle rich text content; and Active Support, a collection of utility classes and standard library extensions that are useful for Rails, and may also be used independently outside Rails.

— From the Ruby on Rails README on GitHub

Rails is also extensible by a wide variety of 3rd party packages. For those unfamiliar with Ruby, 3rd party packages are known as gems, or RubyGems, in the Ruby ecosystem.

Simple Instructions for New Contributors

The project’s How to contribute to Ruby on Rails guide is artfully simple and easy to understand. In comparison to other projects, the contributing instructions are clear and to the point. I believe other open-source projects would do well to follow this pattern of using the CONTRIBUTING .md file to share best practices and expectations for new contributors and save the technical details for another place.

Who Maintains Ruby on Rails?

The Rails project is not part of an open-source foundation such as the Apache or Linux foundation. As described on rubyonrails.org the governance of the project falls to its Core Team, a group of long-term contributors, and a team of supporting contributors known as Committers. The Core Team is responsible for managing releases, handling complaints, evaluating pull requests, and laying the groundwork for new features. The Committer team also assists with pull request review and making changes to code but does not publish releases or set policy.

This is a fairly standard split seen in other open-source projects and in the closed-source, corporate world - generally certain actions are restricted when an engineer joins an organization and they gradually gain keys to the kingdom as they gain experience with the stack and the practices of the organization. Creating a ladder of learning and responsibility for engineers is a best-practice, and the Rails project adheres to this standard with clearly defined roles.

How are issues and pull requests handled?

While the Ruby on Rails pull request template and issue template are quite simple on the Rails project, they cover basic, important necessities. Having been an open source maintainer, I can see a huge amount of restraint here. There is a temptation to pack issue templates full of unnecessary checkboxes to reduce issue spam, but that has not been done here.

There are some rules that the community puts in place to keep the issue list manageable: stale issues are managed automatically and there are some hard and fast rules about support and feature requests not being appropriate in the issue list which are automatically enforced. Discussions of future features are pushed off to the relevant section of the Rails forum. There is also some configuration to handle the situation where the original issue author is not responding.

These are nice quality of life items for the Core Team and Committers. Maintaining an application framework as popular as Ruby on Rails requires a lot of time and energy.

Pull Request Auto-Labeling

Rails has a large codebase, so similar to what we’ve seen in the Apache Spark project the maintainers have set up some tools for automatic pull request labeling to support the review efforts of the contributors. This system adds a tag for each gem that is touched as part of the proposed update. These labels are very useful for maintainers as generally developers on larger projects all have their areas of specialty - few contributors will be an expert for every aspect of the codebase. This allows contributors to filter incoming pull requests to those pertaining to their area of expertise and, when attempting to track down a bug, allows them to filter the list of merged pull requests to identify the culprit(s).

Rejecting PRs Frequently, Kindly, and With Good Reasons

The Rails project at time of writing has 19,136 merged pull requests, and 9,286 that have been closed without merging. This gives a ratio of approximately 1 rejected PR for every 2 merged PRs. The more recent pull requests submitted to the repository seem to have a much higher merge rate, but the high closure rate speaks to the maintainers having a strict approach to rejecting submissions that don’t align with project goals or meet their standard of quality. Looking at recent rejected PRs, we can see that they are handled quickly by maintainers but with an explanation of why the change isn’t being accepted. Here’s a great example:

images/ruby-on-rails-pull-request-code-review-alex-ghiculescu.jpg
From Alex Ghiculescu on a recent pull request review

This is a great approach for rejecting pull requests: polite, with full reasoning. The original author has the ability to verify the validity of the reasoning by following the link to the documentation and doesn’t need to take it on faith.

Conclusion

The Ruby on Rails project has developed mature governance. We can learn a number of best practices from the project including reserving the issue list for true issues, creating a ladder of contribution levels for new authors, showing restraint in ways that make it easier for new contributors to get started, and rejecting PRs frequently, kindly, and with good reason. These best practices can be productively applied in closed-source projects as well - writing good, simple documentation, building context for new hires, and creating an “OK to fail and try again” culture around code submissions.


Find this useful? Be sure to check out these additional articles by Will Barrett:


About PullRequest

HackerOne PullRequest is a platform for code review, built for teams of all sizes. We have a network of expert engineers enhanced by AI, to help you ship secure code, faster.

Learn more about PullRequest

Will Barrett headshot
by Will Barrett

January 12, 2022