This page is dedicated to documenting issues of the code review process. These issues should be backed with data when possible and should always be objective. Please do not propose solutions here (you can do propose solutions here).
If you just want to complain about something, please file a complaint.
See zzolo's post for more context.
Comments
Initial Problems Noted
I just completed by first attempt at a code review. These are my initial impressions:
Repository viewer
The "Repository viewer" link near the bottom of the sidebar of every sandbox allows you to browse the project files before checking them out via git. I generally start there, both to see which core version it targets, and also to make sure it's worth even doing git clone. Many projects have incomplete code, or rely entirely on 3rd party code, both of which mean any further review is not worthwhile.
I agree it would be nice to have earlier indication of core version(s), but I'm not sure it makes sense to do that in the project itself. Full projects already have a system for tracking core versions, so building a new system for sandboxes seems like it would make things more complicate than it's worth. Encouraging applicants to mention core versions in their issue and/or project description might be a simpler solution.
Repository viewer
Thanks, I hadn't noticed that. As long as there's access to the info file, that should be enough. This is more my problem, not a real issue.
Good ideas
Hi @ccardea and @sreynen. Good points.
--
zzolo
Dispute resolution?
Here's an application where reviewers are suggesting removal of duplicate functionality and the applicant is disagreeing with the removal:
http://drupal.org/node/1138978
The discussion has been fine so far; I'm just not sure where to take it from here. Should we have some sort of process to get more people to look at disputes like this?
commented on
Commented: http://drupal.org/node/1138978#comment-4472762
Yes, the best thing to do here is to get more people involved. No one person can be the voice of the Drupal community and say "this is how we do it here", so the more people that can put in an opinion, the better (obviously flame wars are no good).
--
zzolo
Getting a second opinion ...
What do folks think of an official 'Request a Second Opinion' thread on the group's front page?
This would be a thread where, if an applicant is pushing back against the review (typically against a 'duplication' argument, but could be anything), the reviewer can post the application link and request an independant analysis from another party?
Having another individual agree with the reviewer could help mediate applicant pushback; and alternatively, having other reviewers weigh in with a different viewpoint during conflicts could (in time) help ensure greater consistency throughout the overall code review process.
+1
+1
Can the reviewee also request
Can the reviewee also request a second opinion? Seems only fair.
How to handle Features reviews?
I was just looking at this application and I'm a little conflicted on what to do with it:
http://drupal.org/node/1166774
On the one hand, it's new functionality and there's nothing wrong with the code. On the other hand, it's a Features export, so none of the code was directly written by the applicant. This doesn't violate any review criteria I've seen, but it also doesn't give us any idea of the applicant's ability to maintain write or modules beyond Features exports. Thoughts?
There's not so much as a
There's not so much as a hook_form_alter to speak of in there, it is JUST a features export. It's a module in name only. I don't think this should count, as it was created expressly through the GUI (or maybe drush).
Although I think d.o needs a policy on features in general, since to my knowledge none are posted as modules, and there isn't a space for them.
A couple...
There are a couple of feature sets already on D.O. as projects...
The most widely used one from my knowledge is for OpenPublish... http://drupal.org/project/openpublish_features
There others such as http://drupal.org/project/donor_rally_features & http://drupal.org/project/ct_plus
The thing that sets these apart is the fact that they are for specific distributions rather than generic site builds.
One that I've noticed that doesn't link with a distribution is http://drupal.org/project/agreservations
And I've just spotted that "feature packages" ARE included in the list of module categories... didn't know that...
http://drupal.org/project/modules?filters=tid%3A11478
Dreamleaf Media
I think features are fine as
I think features are fine as projects on drupal.org. That's relatively well settled. But I don't think we can really review a project if it is just a feature. I propose we make that a requirement of the process that the proposed project cannot be just a feature export.
knaddison blog | Morris Animal Foundation
Also add 'Install profiles' to that process requirement
Had a similar issue come up two weeks ago, with an application which was a mix of feature, install profile, and custom modules. Eventually had the applicant re-apply with just the custom module component ... but at the time, couldn't find an 'official' policy regarding features as modules on d.o, and the IRC channel could only point me at a post listing public 'feature servers'.
I think this is valid
I think Features is a valid application. It falls within the process goals. And ultimately we are reviewing contributors (or contributions). And if someone just wants to contribute (good, significant) Features, thats a very valid contributor to the community. The focus of the review just shifts more to understanding the applicants ability to understand the contrib space, using Git and versioning, full project description. Code writing itself is not a majority of our current goals.
Also, there are standards for Features (the Kit specification). This would be a good way to review this Feature.
I also think Install Profiles are valid as well. (They usually involve custom code, anyway).
This just speaks up to the fact that we need people from different backgrounds (ie themers) to be doing reviews. Contributions to the contrib repository are not just hand written code.
--
zzolo
Conflicting policies?
This seems to me to conflict with some other project applications in which applicants apply to take over an abandoned module but are ultimately stopped in the project application queue and required to first make modifications to the module before continuing the application process. There is currently an issue in the queue here http://drupal.org/node/1166230 that has this requirement.
The applicant was instructed to provide his or her own code for review because we could not complete the review without seeing his own work. I tend to support this side of the argument as it is, but I do understand that we don't want to exclude beneficial contributions by putting up too many barriers to contributing. Either way, it is clear that we want to provide consistency and a fair and equal review process.
By the way, hello all! Glad to be here.
Not necessary conflicting
"We need to see your code" has always been my understanding, so I was initially very confused by zzolo's response. But after thinking about it more, I don't think it's necessarily a conflict. If we take a step back, the motivation behind "we need to see your code" is, I think, to verify that the applicant is capable of maintaining the project. With Features projects, the ability to maintain them is much simpler: the ability to make and export configuration changes. Unlike regular projects, people can effectively demonstrate this ability without actually writing any code.
It's still a little weird that we have this new set of contributors with a potentially very different skill set than the rest of the contributor community, but that's probably not something we need to address here in the review process.