Project Apps State of the Union

This post reflects my personal opinion and observations regarding Drupal’s Project Application Process, and developments in this space over the past 12 months; with an analysis of the current state (relative to the process mission and goals) and recommendations moving forward.

Tl;dr: Don’t beat up applicants with repeated PAReview scans. For coding standards, the reviewers job is to ‘identify and educate’, not ‘enforce’. When it comes to security and licensing, THEN you can play the role of ‘bouncer’.

Process Mission

The mission of reviewing Full Project Applications is to ensure that new contributors have a basic understanding of the Drupal community's core values concerning contributing code to Drupal.org, AND to promote lasting, long-term contributing to the project.

Analysis

While individual reviewer motivations are all aligned with this mission statement, I feel that our actual implementation of the process is not. In my opinion, execution of the process as it occurs today attempts to drive ‘complete’ understanding instead of ‘basic’, and as a result actually discourages lasting long-term contributions.

Unintended Side-Effects of Automated Review Tools

The introduction of the new PAReview script, CodeSniffer tool, and automation of these tools at http://ventral.org/pareview have undoubtedly revolutionalized the coding standards portion of the Project Application review process. With these tools, any applicant, reviewer, or community member can easily trigger a coding standards review, paste the results in an application thread, and bump it back to ‘needs work’.

Analysis

The ease of this process has resulted in a number of new individuals doing just that, participating in the coding standards portion of the process. This, in turn, has virtually eliminated the ‘time to first review’ for new applicants into the process, which is certainly a good thing.

Unfortunately, this increase in coding standards activity has not been matched by an increase in ‘deep-dive’ manual reviews. Instead, applicants get dragged into a repeating cycle of ‘quick and easy’ coding standards reviews. To make matters worse, subsequent runs often find ‘new’ standards violations which were not identified by earlier reviews as the automated tools mature and evolve. Applicants who have corrected the original coding standards violations are then told to go back and correct new ones … or in some cases, reverse the changes they were initially asked to make.

While the goals of the review process list coding standards as ‘important if they impede the ability to review other elements’, both the repeated coding standards reviews and the level of compliance being pushed on applicants (i.e. pass 100% of violations) exceed this measure, and are not representative of the original intent of the project application process.

Recommendations

Moving forward, and to re-align with the original intentions of the project review process, I suggest the following recommendations:

1. Applicants should not be held accountable to addressing each and every coding standards violation before receiving an RTBC … in reference to coding standards, the job of the reviewer is to ‘identify and educate’, not to ‘enforce’.
2. When new project applications are run through the PAReview script, reviewers should make it clear that the review results are ‘suggested’ improvements, are provided to help educate the applicant with regard to Drupal’s preferred coding standards, and compliance with the coding standards helps to provide a consistency across all projects which in turn simplifies the manual review process.
3. PAReview results should not be posted directly into the application thread, as hitting the applicant with a ‘wall-of-violations’ can be extremely demotivating. Instead, provide a link to the results as stored on ventral.org, or as an attachment in the issue thread, along with some friendly text explaining the items in #1; or, preferably, compile a list of the most common issues and explain each ‘category’ of violation in the application queue, rather than each ‘instance’.
4. Repeated runs through PAReview may be performed in order to confirm that items identified in the first run have been addressed, but subsequent followup comments should be in reference to the original review result, instead of simply posting the output from a new run. (To determine whether an item existed in the original review, use the ‘revisions’ tab at the top of the ventral.org results page.)

Holding applicants accountable to fix new violations which were added to the review tools ‘after-the-fact’, and asking them to correct each and every instance before granting an RTBC, both place the reviewer in the role of ‘enforcer’; which is an area that should be reserved for licensing and security violations only.

Licensing & Security: Where reviewers SHOULD play enforcer

The process goals describe Licensing violations and Security issues as the (only!) two items which should 100% block an application. While in-depth security reviews can be rather time-consuming, a ‘licensing’ check is just as trivial as an automated coding standards review.

Recommendation:

1. Reviewers should make every effort to at least browse the project repository and look for 3rd party code before submitting the project to ventral.org. It is pointless (and frustrating) for an applicant to go through a piece of 3rd party code, modifying the file to meet with Drupal’s coding and name-spacing standards, and then turn around and be told they have to remove the file entirely due to some licensing restriction. Reviewers should take the extra time up front to perform at least a cursory manual review and identify any licensing issues before submitting a project to ventral.org/pareview for testing.
2. Reviewers should make every effort to include a full in-depth manual review with any new posting to an application. Even if the reviewer does not feel they are knowledgeable enough to identify potential security vulnerabilities, anything they do happen to identify will simplify the job for any future deep-dive reviews.

Git Repository structure and branch/tag naming

One item which was introduced with the PAReview script is the all-to-common “it appears you are working in the "master" branch in git” warning. This is almost certainly a ‘Huh?’ moment for full project applicants. Repository setup was never a part of the original review process when it was implemented, and as a result, nowhere is it mentioned or linked to from within the “Applying for permission to create full projects” documentation section.

Recommendation:

1. While not part of the original process, there is a tremendous amount of value in helping new contributors with their first venture into Git. Reviewers should continue to flag repository and branch naming issues (preferably as part of the original cursory review, before a coding standards run).
2. A blurb (with links) about repository structure and naming should be added to the “Tips for ensuring a smooth review” page located at http://drupal.org/node/1187664.