Regarding: http://drupal.org/node/1387232#comment-5574060.
Sometimes I feel that we demand to much of the applicants. That we are to picky, and we make it really difficult for having users access to create full projects. Many of the modules that already is out there have a lots of issues with their code, but we together help to improve it with patches and fixes. We can not demand that every sandbox project should meet standards that the most of the modules does not do that are already out there.
Mostly of the big projects have a lot of issues, take a quick look at Views, Token and Pathauto, three of the most installed modules, they have a lot of coding standards issues for example. That does not make them bad or says that merlinofchaos, eaton or Dave Reid does not understand the Drupal coding standards.
We need understanding of coding standards, security, documentation etc. Most of the applicants get the really quick if they do not have it from the beginning.
And yes, I think some of the us are to picky, and takes the fun out from contributing to the community.
Comments
Just for fun I did automatic
Just for fun I did automatic code reviews of all the Code Review administrators main projects, and all have coding standards issues. And it is nothing wrong with that. We are all humans (I guess ;-)). Everybody can get better, but we should not hold back each other when we should help in pushing forward instead.
/* Mikke Schirén, https://digitalist/ */
The mentioned application was
The mentioned application was bounced due to a security issue primarily, and two not so critical error/strange behaviour causing issues.
Yes, I do think we can be too picky, but not on security issues.
While I was reviewing, I also pointed out a number of non-blocking issues which if there had been no blocking issues, would have simply been there are "things to look at" either before or after launching the project. They do not stop the project from moving forward.
The one thing I consider being too picky, is bouncing a manually reviewed RTBC application because of an automated coder review. That is much better saved for an issue on the sandbox/project and instead doing the normal review and fixing it if it's ready. The coder module has gone so far that I no longer use it on reviews unless the code is unreadable and instead just mention that they should ready the coding standards and run coder themselves.
Expecting people to write secure code, without obvious logic errors in it is not demanding too much from applicants. The full projects are meant to be release ready code. A sandbox will suffice for a project before it is at that stage in most, if not all circumstances. The reviews should easily get that sandbox code up to a point where it is ready to be a full project and released, all the while demonstrating the abilities and shortcomings of the applicant to the point where they will be granted full git vetted status.
Just because there are projects already in circulation with errors in them does not meant that we ignore all errors in everything else. That will not improve anything. I do mean code errors here, not coding standard issues. I've submitted patches and contacted authors of modules where I had run into serious enough issues that they needed that kind of action, but we should also be ensuring that everyone new coming along also knows what they need to be aiming for. The number of people who submit applications without having ever read any of the supporting documentation is astounding; so much so that we have cookie cutter responses to paste onto reviews.
If an application has serious enough issues that it needs work, it will be set to that.
Also, you have to be much
Also, you have to be much more strict with the first module, since the purpose of the review process is not to check if the module works but instead if the developer is aware of all the coding standards and potential problems.
I do not agree on that. I
I do not agree on that. I think the applicants must be aware of the all the issues, and solve the really important ones, but I do not think everything need to be solved before it makes it to become a full project. The strictness have no meaning if it just about getting the automatic tests to show nothing is wrong. You could make that without understanding why.
EDIT: Just a note - should not the reviewer test if the module work? Some of the problems with the projects in the application queue I have not noticed until I installed it.
/* Mikke Schirén, https://digitalist/ */
My comment about being picky
My comment about being picky in that issue were before you mentioned the security issue - security is important, and that should we not negligent.
I just started reviewing for a couple of weeks ago, and in the beginning I were really lost, and overused automatic reviews, but I think in general we need to have a better documentation on how to do reviews, especially for beginners on reviewing. Hopefully http://groups.drupal.org/node/207708 could be a good start for that.
/* Mikke Schirén, https://digitalist/ */
The security issue and
The security issue and associated other blocking issues were mentioned, under the h3 title "blockers". The word "security" was not expressly used, so it perhaps could have been worded better, but from the text it reads like a security issue. After all, it was serious enough to block it.
207708 looks like a very sensible approach to the coder review module, and additional guide for reviewing. As I stated before, I find using the coder review module to bounce applications after they have been manually reviewed something to be avoided, unless it's bad enough that it becomes an actual issue again.
I tend to base reviews more on these links the pages underneath them:
My interpretation
I can certainly see where both of you are coming from on this, and wanted to add my two cents.
When reviewing projects prior to granting the 'Full Access' permission, our job as reviewers is to be thorough in our discovery, and strict on licensing/security. The intent is to identify areas where the applicant may be lacking knowledge of the 'Drupal way' of doing things, and help to guide them in building that knowledge. There is nothing wrong with being 'too picky' when identifying potential areas of improvement.
Where the problem arises is in the manner we communicate these results back to the applicant, which affects their interpretation of the feedback and perception of the overall process as a result. We should be picky when reviewing, but need to massage our feedback in such a way to avoid reinforcing the 'applicant versus reviewer' perception ... in other words, we enforce via recommendations and suggestions, not demands. We have to be picky, but avoid nit-picking. We need to be strict in identifying any security issues, but also thorough in educating the applicant as to why and how to avoid the same mistakes in the future.
In my mind, this is by far the most challenging and overlooked aspect of the code review process ... how to play gatekeeper without being interpreted as a roadblock. For me, it all starts with what we post in the issue queues. We should be careful to avoid comments such as 'You need to ....' or 'This is wrong ...' or anything that looks like a demand; a change in phrasing may be all it takes to reduce the 'picky' factor without actually lowering the standard.
It's all good!
Enjoying the process! This is the time to do it and I appreciate it!
It is very useful and I thank everyone for their contact time. Every point is helpful. Feel like I am on an exponential curve up in learning the 110% right way to do things in Drupal.
Cheers,
~Darrell
Drupal StackExchange