We need to clearly define the goals of the Full Project Applications process (and possibly name it better). There is currently some outlined on the how to review. Also, see zzolo's post on overall way of building 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.
(zzolo's opinion) The long-term contributing part is important, as it says we want to build a community, not just we want your code.
Specific Goals
The following are specific things to review for and keep in mind while reviewing.
Critical
These items are things that will 100% block an application.
- Licensing: Code/images copied from other sources must be licensed GPLv2 and later. (needs links)
- Security: We should try to catch all security issues in this stage AND educate the developer about them so they won't make more of the same mistake.
Important
The following are more subjective goals to review for.
- Module duplication: if possible reduce module duplication, but if a maintainer believes their module is different in some way then accept their word.
- Best practices and coding standards: mostly important if they impede the ability to review other elements
Values
The following are important things to keep in mind while reviewing as they create better, longer lasting contributors.
- Mentoring: view this as an opportunity to teach rather than one to criticize.
- Welcoming: these are the rockstars of Drupal's future. Bow down before them and welcome them.
- Should we be including the DCOC in here somewhere?
Comments
Add functional testing to goals?
I initially raised this issue on the solutions page, but someone noted that finding out whether or not a module actually works is not listed as one of the goals of code review. Based on some of the responses there, it looks like there needs to be some discussion of what level of functional testing needs to be done. There seems to be some consensus that requiring simple tests is too high of a barrier, but there is some disagreement on whether it is necessary to actually download and install a module to see if it works as advertised. It has been suggested that if you know the API's well enough, you can tell if a module will work by looking at the code. In my opinion, that will only tell you that the module should work. Case in point, I downloaded and installed a module that had an apparent syntax error that caused a white screen. Chances are you would not find that just by looking at the code. The question is, how much should be done before a project is approved, and how much can be left to the issue queues afterward?
Basic understanding
I still stick with not installing modules. Whether it works or not, my opinion of the goal of the review is to determine if the reviewee understands Drupal basics to be a good contributor. I don't write perfect code and break stuff all the time, but I understand how I should be doing it when I do get it right.
--
zzolo
Basic understanding
I generally respect your opinions zzolo, and I am the new guy here, but let me just point out a few things from the project application page.
The indication there is that the project should be both working and release ready, It seems to me that a reviewer hasn't done his job if he hasn't verified that those requirements have been met. Then again I'm new here so maybe my question should be, what kind of release are we talking about here? Is it possible for a newly approved project to be tagged as an alpha or beta release, as opposed to a stable release? If that is the case then maybe the application page should be changed to reflect that.
From a user's point of view it's important to know what the state of the project is. If a user downloads a project and it doesn't work as expected, then you've wasted his time, caused frustration and a negative perception of Drupal.
good point
Hi @ccardea. Even if you are the "new" guy, your opinion is important.
You make an excellent point. These are not good words to be using. We need to update this wording, reflecting the goals that are figured out here. I think it would be best to let the goals get fleshed out for a little while (a month or two), then we can be a lot more confident making changes to that documentation page.
--
zzolo
Define the basic level of quality expected
Actually I was being a little facetious when I said that the wording should be changed.
From the old CVS text, posted by bonobohere
You must submit a finished, working module or theme for review along with your application
From bonobo's wiki page
Code review of existing contributed modules is frequently cited as a means to improving the quality of existing contributed modules.
The Problem: Many contributed modules have had little to no outside review. This creates widely varying quality within the contrib space.
The Solution: Get more people involved in reviewing modules.
Bonobo defined the problem with quality as being widely varying. This is different from poor quality that needs to be improved. If quality is widely varying, the possible solutions are:
Number one is not practical, so number two would seem to be the solution, but first we have to define what we mean by quality. There are two basic concepts that have been used consistently in the documentation to define quality. The first of these is "working". What do we mean by "working"? This has to be defined in terms of the functionality that the module provides. I propose that this would be the definition of a working project:
The other terms that are used to define quality in the documentation are "finished" or "release ready". I propose that "release ready" should be defined as
Regarding 'finished'
I've only been lurking for a few days, but I'd have to agree that the 'working' and 'release-ready' requirements (as listed on the project applications page) are important components of the code review process. 'Working' in the sense that it does what it's supposed to do, without throwing out a bunch of warnings and issues; and 'release-ready', in that it meets the various best practices as per ccardea's post. These are essential to maintaining quality, and the steps in getting to this stage help contribute to the 'mentoring' process.
However, I think the 'finished' label is one that could be relaxed (or should possibly be avoided). If a developer comes up with a new module, but has only accomplished the first 30% of his planned functionality/roadmap, the term 'finished' implies that they should complete the other 70% before applying for full project status ... I'd propose that, if the functionality provided in that first 30% is able to 'stand alone' and provide value in and of itself (ie. that 30% functionality itself is 'finished'), the developer should be encouraged to submit it for review.
As the contributor of a relatively large project (~80k loc in 12 sub-modules), I delayed my application for months while I plugged away on additional features, so that I could ensure I was submitting a 'finished' module. As a result, I expect the review of my application is likely to be a fairly labour intensive process for a reviewer; and any mistakes I was making early have now been propogated throughout the entire module. Had I submitted just the base module and one or two sub-modules early in the process, these mistakes could have been pointed out earlier, and the effort required for a full code review would be much smaller ... but as a newbie to the process, I locked on to that 'finished' term. (That, and the advice I received in IRC was to submit the 'complete' package.)
I disagree
@greggles, I disagree with your move of things that should not block.
Though, I don't think a module should be stopeed if it sort of duplicates another module, but I think its very important to show that the reviewee understands the Drupal module ecosystem and has put in some effort to look for similar modules. The idea is to encourage the reviewee to not duplicate code and efforts as that hurts the community and I think this is a pretty core value that our community holds.
And I think coding standards and best practices are important as well. Again, its not that a module has to pass Coder, its about demonstrating knowledge of the best practices. I think following coding standards and best practices is a pretty core value of the Drupal community as it allows us to collaborate much more effectively.
--
zzolo
Based on the way you've
Based on the way you've stated it here, we mostly agree.
I've seen several applications get stuck because:
In both of those cases I think the Drupal world is better served to let the module and applicant get approved and hash out these details after the fact. Too often I see the process as something used to require perfection of an applicant and that's neither realistic nor productive for our community. For some reason these two points seem to be the most contentious and a major source of frustration for new contributors.
I think especially the comments in this thread point toward an even less restrictive process than we have now.
My goal in organizing this wiki the way I did was to say:
knaddison blog | Morris Animal Foundation
In both of those cases I
I disagree -- this is perhaps true in some cases, but often this just serves to add yet another lego brick to the Huge Heap of lego bricks.
also the mason
I can see where this perspective comes from, but I think it's far too limited, only looking at the immediate benefits of someone getting through the approval process, a very small fraction of the long-term benefits. Specifically, we're approving a contributor, not a single project. It's true we use a single project as the basis for that approval, but most contributors end up contributing far more than just that single project, including other contrib projects, core updates, documentation, bug reports, and so on. It's too easy for the approval process to look like arbitrary hoop we're asking new contributors to jump through, and cause otherwise valuable community member to give up in frustration. Unfortunately, we know this happens.
I'm certainly not suggesting it isn't important to keep out bad contributions, but we need to consider that risk on balance with the risk on the other side: losing good contributors. It's far from just another brick we're getting with approval; it's also the mason who made it.
Perhaps it would help to take
Perhaps it would help to take another step back and re-examine one of the underlying assumptions in front of this debate ... being that the current process ties approval of the 'contributor' to successful approval of the 'module'. There is a perfect example of this at the top of the review queue this week, with Nylin's Term Delete Replace application. If a contributor's submission successfully demonstrates adherence to best practices, knowledge of the APIs, a willingness to learn, the patience to listen and implement suggested improvements, and dedication to stick with it throughout the whole code review process; I'd say that they've demonstrated all of the traits you're looking for in a contributor ... even if the review process itself leads to a conclusion that the module's functionality would be better served elsewhere (whether due to duplication, existence of a parallel initiative, or the suggestion of a more efficient approach to the same task).
If the only outstanding issue with an application is that the module itself is a duplicate, then don't send the contributor away with a message that you'll fast-track their next application ... to me, this reads a little too close to 'better luck next time'. If they have demonstrated the competence to justify a 'fast-track' promise for their next application, then simply deny the module 'contribution', and grant the 'contributor' role.
Granted, the example provided is probably more of an exception than the typical code review scenario, and I don't suggest that deny module/accept contributor results should become a common occurence; but the process needs to be flexible enough to accommodate this option when it does arise.
If a contributor's submission
That's exactly the point I have been trying to make -- but you have put it so much better than I was! Thank you :D
As much as I agree with this,
As much as I agree with this, it is very difficult to create a structure around defining the ability to have a "willingness to learn, the patience to listen and implement suggested improvements, and dedication to stick with it throughout the whole code review process".
We need to be able to say these things in a more Drupal specific way. Such as have the "knowledge of existing related projects and demonstrated ability to compare and integrate where relevant". IMO, this basically implies most the above statement, but can be much more objective and measurable.
The "dedication to stick with it" is simply demonstrated by a completed application.
--
zzolo
Patience is limited.
I've had lots of patience and I feel like I'm very dedicated to the Drupal project. My application has been in the queue for 15 weeks now. In starting to get formally involved in the "code review process" to help fix this wait by reading these comments, offering suggestions, reading applications, commenting on them. Having done so I'm becoming less patient with the process.
Also, a note to this quote from @greggles:
"After fixing 10 things in a module that otherwise passes a style review, a reviewer notices one small code style issue and the maintainer just never responds."
The thing with the small issues is that they are small. I want to get my module moving and I'm more than happy to fix the small stuff... but now because the last response I had was to fix a few minor (very minor, IMO) things I now get to add +4 weeks to my waiting time.
During this time I'm still responding to user requests and also having to manually zip the files for them each time because they don't know git. Awesome.
I certainly hope that at week 19 I'm not faced with a comment to remove extra line breaks so that I have to wait until week 23, or week 27, or next year to get approved.
Small vs. big
Small issues are indeed small, but the issues in your application are not all small issues. One opens Drupal.org to legal liability, and the other will bring down entire sites on some servers. Not exposing less technical people (i.e. those unfamiliar with Git) to problems like these is exactly why we have this review process.
The line break issue, on the other hand, is small, and your reviewer said that when he mentioned it. If that was the only issue in your application, it would be approved already.
4 weeks between feedback is the maximum wait (at least today), not the minimum. Sometimes you'll get a response the same day. But volunteer resources are spread thin, so sometimes it takes longer. You can help speed up the entire queue by doing reviews yourself.
I commented in the issue, as
I commented in the issue, as did Scott. All I can say about the timing is we are doing our best to get your review done as quickly as possible now. We can't do anything about the weeks that have already been lost. I don't imagine your module will take long (from this point.) And as was pointed out, it is 4 weeks until critical, not 4 weeks for a response.
Hi @Ryan. I do feel your
Hi @Ryan. I do feel your pain and am sorry for the wait. But as noted by others, its all volunteer.
Also, this thread is not really the place for this sort of discussion. We do very much value your feedback. As part of this group, there is a place to file complaints. If you want to chime in on what the goals of this process should be, your input is valuable.
--
zzolo
It depends
I think there are cases with both duplicates and coding standards that should go through, but also cases that should be stopped. I'm not sure how or if we could formalize those into less subjective standards, but it might be worth going into a bit more detail on those points.
For duplicates, I generally start by asking the project owner to explain the differences from existing projects and what, if anything, they did to try to work with the existing projects. If the answer to that is something along the lines of "oh, I didn't know that project existed" or "that project is old, this one is new," those seem like good reasons to push for collaboration. If, on the other hand, they're able to explain core differences in functionality and/or point to an issue where those differences were rejected from the existing project, I think that should definitely go through. For me, it's a question of the project owner's approach to collaboration more than the project code itself.
For coding standards, anything that only affects readability, I mark as a minor issue and move on. I suppose if something make the code completely unreadable, I might let that hold up the application, but I've never seen that. Coding standards that actually impact functionality or performance, though, I think are worth moving to "needs work" and have the owner at least respond to, if not fix, the issue before going forward.
In my experience, the only time this kind of stuff really holds up applications is when the project owner doesn't respond at all, and I think that has more to do with them not noticing their application has been updated than the requirements being overly onerous.
I'd like to see criteria to
I'd like to see criteria to do with how the proposed module fits within the Drupal ecosystem, how it adds to it, and how it interacts with it.
Eg:
could you expand
Hey @joachim. Thanks for your thoughts. Here are some follow up questions:
--
zzolo
A long shot ...
As far as goals go, I think 'nirvana' would be changing the "project application" process to a "mentor request" process ... with the idea being that applicants would get paired with a mentor before the actual development cycle; and the mentor would be largely responsible for blessing the project upon completion.
I know, there are hundreds of issues with this approach right now; but as far as a 'long-long-long-term' goal, it may be something to keep in the back of our minds.
Proposal: remove module duplication from our goals
Collaboration is one of the key principles of Drupal. As a community, we strongly value working together on a single project over competition between projects. I'm not suggesting duplicate projects is not a problem, and would encourage anyone who thinks that to have that debate elsewhere.
The question I'd like to pose: is the new contributor review process a good place to address this problem? I think the answer is probably no.
At best, this review process will only catch duplicates on the submitted project. If that particular project doesn't happen to duplicate functionality, the topic never comes up, so we're doing nothing to discourage duplication on subsequent projects. Because the wider community already needs to deal with duplication independent of full project reviews, removing it in the review seems unlikely to make the problem substantially worse.
On the other side, removing duplication checks would, I think, make reviews substantially better. Duplication concern is, in my experience, one of the most time-consuming issues. Project creators hate being told they shouldn't have invested all this time into what they created, so it almost always becomes a long back-and-forth exchange with reviewers carefully (and slowly) crafting messages that enforce a hard-to-define rule, but avoid giving the wrong impression of how our community values contributors. It's incredibly difficult to mentor or welcome in this context, and it feels like a waste of time dealing with licensing or security issues if the application seems likely to be rejected over duplication.
I'm not suggesting we should avoid mentioning duplication where we see it, just than we shouldn't change the status to "needs work" or otherwise slow down an application when we do mention it.
Thoughts?
Agreed ...
Well said ... and excellent point regarding this not catching 'second module' duplication if the first isn't a dupe - I hadn't considered that angle before you mentioned it.
My interpretation of the dupe check with respect to code review goals is that it should be a "Hey, have you considered?" question which is asked during the review ... and would suggest that it should be left in the review process, but by no means treated as an application stopper unless there are already MULTIPLE other module options with the same functionality.
Leave it in to catch opportunities to funnel applicants over to the 'abandoned modules' process ... but do not make it a requires justification gate.
It shouldn't block or stall applications
"we strongly value working together on a single project over competition between projects" well, to the extent that "competition" is real, I agree. I've mentioned previously that I don't believe that duplicate modules have to be regarded as "competing" - that's a human (or community) attitude, not a code (or repository) attribute. Other communities appear (to me) to do very well with just letting the contrib extensions speak for themselves and letting the users vote with their feet. I've described ways to help users choose for themselves between what might appear to be overlapping or duplicate contributions.
"wider community already needs to deal with duplication independent of full project reviews, removing it in the review seems unlikely to make the problem substantially worse." Agreed, and, agreed!
"Project creators hate being told they shouldn't have invested all this time into what they created" They have submitted code and volunteered for the "mentorship" opportunity. Denying this opportunity is contrary to our goals because dupe review is not code review.
Certain code-reviewers might feel like reviewing code which duplicates another project is "a waste of time [...] if the application seems ...[like a]... duplication", but it certainly shouldn't be the basis for rejecting the application.
We want to grow, not diminish, the enthusiasm, to enable, not bar the opportunities, for "joining" the Drupal project as a contributor, and, yes, as a code reviewer too. If "senior" (not to say "jaded") code-reviewers want to personally skip these ones, newer code-reviewers remain available - we certainly do have people looking for "getting started in code-review".