Pull request review guidelines

Every change made to Kibana must be held to a high standard, and while the responsibility for quality in a pull request ultimately lies with the author, Kibana team members have the responsibility as reviewers to verify during their review process.

Frankly, it’s impossible to build a concrete list of requirements that encompass all of the possible situations we’ll encounter when reviewing pull requests, so instead this document tries to lay out a common set of the few obvious requirements while also outlining a general philosophy that we should have when approaching any PR review.

It is not expected nor intended for a PR review to take the shape of this document, where the topmost guidelines are always applied first and the bottommost guidelines are always applied last. This entire document should be considered as a whole rather than as a series of consecutive todos that must be completed in order.

While the review process is always done by Elastic staff members, these guidelines apply to all pull requests regardless of whether they are authored by community members or Elastic staff.

The target audience for this document are pull request reviewers. For Kibana maintainers, the PR review is the only part of the contributing process in which we have complete control. The author of any given pull request may not be up to speed on the latest expectations we have for pull requests, and they may have never read our guidelines at all. It’s our responsibility as reviewers to guide folks through this process, but it’s hard to do that consistently without a common set of documented principles.

Pull request authors can benefit from reading this document as well because it’ll help establish a common set of expectations between authors and reviewers early.

Every pull request is different, and before reviewing any given PR, reviewers should consider the optimal way to approach the PR review so that if the change is ultimately rejected, it is done so as early in the process as possible.

For example, a reviewer may want to do a product level review as early as possible for a PR that includes a new UI feature. On the other hand, perhaps the author is submitting a new feature that has been rejected in the past due to key architectural decisions, in which case it may be appropriate for the reviewer to focus on the soundness of the architecture before diving into anything else.

There are a lot of discrete requirements and guidelines we want to follow in all of our pull requests, but three things in particular stand out as important above all the rest.

Reviewers are not simply evaluating the code itself, they are also evaluating the quality of the user-facing change in the product. This generally means they need to check out the branch locally and "play around" with it. In addition to the "do we want this change in the product" details, the reviewer should be looking for bugs and evaluating how approachable and useful the feature is as implemented. Special attention should be given to error scenarios and edge cases to ensure they are all handled well within the product.

Having a relatively consistent codebase is an important part of us building a sustainable project. With dozens of active contributors at any given time, we rely on automation to help ensure consistency - we enforce a comprehensive set of linting rules through CI. We’re also rolling out prettier to make this even more automatic.

For things that can’t be easily automated, we maintain a style guide that authors should adhere to and reviewers should keep in mind when they review a pull request.

Beyond that, we’re into subjective territory. Statements like "this isn’t very readable" are hardly helpful since they can’t be qualified, but that doesn’t mean a reviewer should outright ignore code that is hard to understand due to how it is written. There isn’t one definitively "best" way to write any particular code, so pursuing such shouldn’t be our goal. Instead, reviewers and authors alike must accept that there are likely many different appropriate ways to accomplish the same thing with code, and so long as the contribution is utilizing one of those ways, then we’re in good shape.

On the other hand, if a reviewer calls out a specific chunk of code as being hard to understand, then one of the first people to look at the code is having a hard time understanding it. If the reviewer feels strongly that this is a problem, they should raise the issue for the author’s consideration and potentially propose alternatives. The reviewer must be pragmatic, and their goal should be to propel the discussion forward rather than simply throwing up obstacles.

When in doubt, relying on "prior art" in the codebase, especially in and around the area of the codebase that is being contributed to, is generally a good idea.

There may also be times when a person is inspired by a particular contribution to introduce a new way to style code that we already have different style guidelines or "prior art" for. It’s OK to bring this up in a pull request, but ultimately that discussion should branch off into a separate issue or pull request to update the appropriate guide. If this change is prompted by a reviewer, then the original PR should not be blocked on this. If the change is prompted by the author, then they can either update the PR to be consistent with our existing guidelines (preferred) or they can choose to block the PR entirely on that separate styleguide discussion.

Nitpicking is when a reviewer identifies trivial and unimportant details in a pull request and asks the author to change them. This is a completely subjective category that is impossible to define universally, and it’s equally impractical to define a blanket policy on nitpicking that everyone will be happy with.

Reviewers should feel comfortable giving any feedback they have on a pull request regardless of how trivial it is. Authors should feel equally comfortable passing on feedback that they think is trivial and inconsequential.

Often, reviewers have an opinion about whether the feedback they are about to give is a nitpick or not. While not required, it can be really helpful to identify that feedback as such, for example "nit: a newline after this would be helpful". This helps the author understand your intention.

Conflicting opinions between reviewers and authors happen, and sometimes it is hard to reconcile those opinions. Ideally folks can work together in the spirit of these guidelines toward a consensus, but if that doesn’t work out it may be best to bring a third person into the discussion. Our pull requests generally have two reviewers, so an appropriate third person may already be obvious. Otherwise, reach out to the functional area that is most appropriate or to technical leadership if an area isn’t obvious.

Whether or not a bit of feedback is appropriate for a pull request is often dependent on the motivation for giving the feedback in the first place.

Demanding an author make changes based primarily on the mindset of "how would I write this code?" isn’t appropriate. The reviewer didn’t write the code, and their critical purpose in the review process is not to craft the contribution into a form that is simply whatever they would have written if they had. If a reviewer wants to provide this type of feedback, they should qualify it as a "nit" as mentioned in the nitpicking section to make it clear that the author can take it or leave it.

Inflammatory feedback such as "this is crap" isn’t feedback at all. It’s both mean and unhelpful, and it is never appropriate.

Establishing a comprehensive checklist for all of the things that should happen in all possible pull requests is impractical, but that doesn’t mean we lack a concrete set of minimum requirements that we can enumerate. The following items should be double checked for any pull request: