WP-CLI is the official command line tool for interacting with and managing your WordPress sites.
Need help with something? Please review your available support options.
Want to help make it better? Check out our Contributing guide for an introduction, or “good first issues” for your first pull request.
EditCode review is a core part of the WP-CLI project’s software development workflow. Code review and pairing on code are necessary to maintain quality output, as well as build cohesive styles in our work. Having a well-understood and cohesive style can make all our work easier to approach and maintain for new as well as established developers. If done right, code review can also be a learning process for all involved. Here’s a typical code review workflow:
We’re currently using Github for everything. Read up on our Github workflow for details of process, naming, usage of issues and pull requests. A moderately complex issue will typically be addressed across multiple pull requests, each tackling a distinct part of the issue. This makes review simpler, because each review stage will be looking at a small code.
@wp-cli/committers for review.A good way to visualize the objectives of code review is this analogy to Maslow’s “Hierarchy of Needs” pyramid. From most basic to the highest level, a reviewer is checking that code is Correct, Secure, Readable, Elegant, and Altruistic. It’s important to keep this sense of priorities: if a change introduces unhandled edge cases, bugs, or security vulnerabilities, those issues need to be addressed before coding style guidelines or beautification practices preferences will matter.
As a reviewer, your first job is to get an understanding of what the proposed change does and why it’s essential. There’s no point in critiquing anything until you understand what it does, why it’s necessary, and what decisions went into the way this was built.
Next, look it over for correctness. Are all functions which take parameters and produce output covered by functional unit tests? Do they actually do what they’re supposed to? Can you picture an edge case where a function would error unexpectedly or return something other than the expected result? If this command has output, does it render properly in all situations? Are all global flags handled? Does it have unsurprising fallbacks for uncommon situations? Does it handle errors with clear output messages?
If it addresses the business and UX requirements, the next thing to check for is security. Does it follow basic sanitization and escaping practices for all untrusted input? If it interacts with other aspects of the codebase, is it liberal in the inputs it accepts and conservative in its output, making sure to only pass expected values? Think like an attacker. If there’s any way a malicious agent could exploit this code, or an unlucky user could trigger a bug that fatals or looks bad, it’s your job to find it.
Next, check for readability. Functions, variables, and files should be named clearly according to their meaning. Everything should adhere to the surrounding code style.
All code in any code-base should look like a single person typed it, no matter how many people contributed.
As readability is inherently subjective, this requires the ability to look back from the immediate code changes to the bigger picture. Think of someone months down the road trying to trace a given code path through the current changeset. Are there unnecessary steps that could be simplified? Are code comments and inline documentation robust enough to recreate the thought process behind the code?
Finally, check for elegance and overall quality. Code should follow existing and known patterns, so that others can understand it at a glance. If a change introduces a chance to refactor surrounding functionality, to abstract and standardize old code into new patterns, suggest those opportunities.
As the person receiving the code review, your job is to learn from suggestions. Defensiveness, stubbornness, or impatience can prevent you from getting the most out of suggestions. It’s OK to explain yourself if you feel the reviewer seemed to misunderstand your intent. Arguing semantics or insisting on the correctness of one approach is almost always a bad habit and distracts from the team process. If something isn’t clear to someone familiar with the codebase who’s reviewing your work today, it will definitely be unclear to a new developer being on-boarded six months from now.
It’s best to respond to the issues addressed by the reviewer as quickly as possible. Keep in mind that the review process requires context switching on the part of the reviewer as well as on your part, and the more immediate that process is, the less disruptive that context switch will be.
Glen Sanford: On code review – “Pending code reviews represent blocked threads of execution[, code review should always be your top priority]”
The Ten Commandments of Egoless Programming – “Understand and accept that you will make mistakes. The point is to find them early, before they make it into production.”