A Drupal 6 module:
The Droogle Module integrates Google Docs with Drupal. It provides a list of a configured google user's documents and allows uploading docs to that account. It also exposes much of the google docs api to Drupal users -- you can create collections, move documents into collections, etc. Droogle also works with Organic Groups and each Organic Group can hook up to a unique user account to manage it's group documents. The module's creation is sponsored by Babson College and is to be used to better manage documents for groups, classes, etc. Integrating documents with google means better access to documents on ipads, iphones, etc, since there are many apps for these devices to sync your google docs account(s). Droogle is not to be confused with BBoogle, which is Blackboards integration with Google Docs -- but Droogle works much the same way and serves the same purpose.
We are looking for ideas on how to improve the module to make it as useful as possible in eduction and to the general community. We are open to having co-maintainers, preferably with another college or university.
http://drupal.org/sandbox/barnettech/1327544 (project sandbox page)
http://drupal.org/project/1327544/git-instructions (link to git instructions for the project)
http://drupalcode.org/sandbox/barnettech/1327544.git (link to the project in git)
There is another project that integrates with google but it didn't really work, we tried it, and it didn't work in Organic Groups, and didn't expose a lot of the functions to create users, work with google docs folders, etc. This module is much more plug and play and covers more of the google api. There is also a theme function to wrap google document lists.
Here is a screenshot: http://www.barnettech.com/content/i-will-be-releasing-droogle-module-sho...
REVIEWS I HAVE DONE:
1.) http://drupal.org/node/1379794#comment-5495068
2.) I did a number of reviews in this thread: http://drupal.org/node/1316852#comment-5348664
3.) this thread as well I did a few: http://drupal.org/node/1166144#comment-5347174
4.) http://drupal.org/node/1236914#comment-5517848
5.) http://drupal.org/node/1354558#comment-5517898
6.) http://drupal.org/node/1336946#comment-5518252
7.) http://drupal.org/node/1336946#comment-5519404
8.) http://drupal.org/node/1336946#comment-5523648
9.) http://drupal.org/node/1336946#comment-5524046
10.) http://drupal.org/node/1336946#comment-5524806
11.) http://drupal.org/node/1236914#comment-5557230
12.) http://drupal.org/node/1379794#comment-5557258
Comment | File | Size | Author |
---|---|---|---|
#55 | drupalcs-result.txt | 1.9 KB | klausi |
#47 | drupalcs-result.txt | 955 bytes | klausi |
#42 | drupalcs-result.txt | 44.77 KB | klausi |
#5 | pareview_result.txt | 132.04 KB | doitDave |
Comments
Comment #1
klausiZend: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
Comment #2
Barnettech CreditAttribution: Barnettech commentedThanks. I figured the Zend code was open source as well. Does that not matter?
Comment #3
Barnettech CreditAttribution: Barnettech commentedOk I've removed the Zend directory and updated the README file on where folks can download the latest Google Gdata api. The readme gives exact instructions on how and where to put the Zend directory.
Let me know what else needs doing.
Thanks,
James Barnett
Senior Drupal Developer / Architect
Babson College
jbarnett@babson.edu
Comment #4
klausiyou need to change the status if you want a review.
Comment #5
doitDave CreditAttribution: doitDave commentedI have attached automated review results for your convenience. Did you already check with http://drupal.org/node/1011698 - especially the recommended lectures such as coding standards?
Manual review:
Comment #6
Barnettech CreditAttribution: Barnettech commentedI'm amazed that drupal.org rejects hard work and sharing of code for minor errors like:
"Line indented incorrectly; expected at least 2 spaces, found 1"
I don't have time to look at such errors for a week or 2 at least. I actually had run it through the coder module already actually and found there were no major errors.
Do folks really treat every minor thing for submission here? I will comply if needed but it will greatly slow down my ability to share. I will put the code on a branch
though, I agree that needs doing ... thank you. Let me know if the "Line indented incorrectly; expected at least 2 spaces, found 1" kinds of things will hold up this
getting accepted.
Comment #7
Barnettech CreditAttribution: Barnettech commentedI've made a tag for this, so this should be all set. I've also confirmed that most other major drupal modules that are released as full projects have these messages such as: "Line indented incorrectly; expected at least 2 spaces, found 1"
I did run it through the coder module and all major problems were resolved.
In CTOOLS for example in the drupal 6.x version I'm seeing :
Comment #8
jthorson CreditAttribution: jthorson commentedbarnettech,
Admittedly, the automated tools which are being used to review applications can be a bit nit-picky ... however, these tools are also revolutionizing the project review process, helping us provide feedback within hours or days of a new application relative to the weeks that it often took before their introduction. These tools flag both 'blockers' and 'suggestions' ... we don't expect every application to be perfect, but we do strongly recommend conforming to the established Drupal coding standards, to maintain code/formatting consistency within the contrib space.
In time, we hope to fully automate the initial review of new project applications using similar automated review tools ... in the meantime, there are some key items hidden within the coding style warnings which will need to be addressed before your application can be approved. Addressing the coding style items while tackling the big-ticket items will help both you and potential reviewers more easily filter out those big ticket items from within the review script output on future follow-ups.
There are multiple reasons behind not including 3rd party libraries. The licensing compatibility (and legal implications) is only one of these. (Incidently, anything released on drupal.org is automatically licensed under GPLv2 ... so unless the 3rd party library is also GPLv2, it's a non-starter.)
Another reason is to avoid conflicts when multiple modules attempt to include the same 3rd party library within their code. This can lead to name-spacing conflicts between functions, as well as issues where two modules try to include two different versions/releases of the same library. To avoid these site-killing issues, we recommend that all 3rd party libraries be placed in sites/all/libraries, and leverage the Library API for inclusion.
From Docs.php:
If it's subject to BSD, it can't be hosted on drupal.org.
Other items:
Menu item titles and descriptions should NOT be enclosed within t().
Because the code already runs these through t() internally, the t() call in your code redundant.
There should be no trailing spaces
Extra whitespace triggers error messages when others create patches for your module, affecting readability (and reviewability) of the resulting patches.
The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
Self-explanatory.
Function name "getClientLoginHttpClient" doesn't include package part.
Even if it is only example code, your example should properly prefix function names so that folks duplicating your example code can avoid function namespace collisions. Other files in the module also use generic names such as 'uploadDocument' ... prefix these with your module name to ensure a it's unique across an entire site.
Remove all old CVS $Id tags, they are not needed anymore.
This is a leftover from CVS, and are redundant now that we've moved to Git.
Please ensure that these have been addressed and uploaded to the sandbox repository before setting back to 'needs review'. And to avoid another thousand-line automated review output, I'd *strongly suggest* adjusting your indentation to the Drupal standard of two spaces (no tabs).
Comment #9
jthorson CreditAttribution: jthorson commentedComment #10
Barnettech CreditAttribution: Barnettech commentedThanks for your comments. I just updated my code to fix lots of the items listed above, how can I run an automated report as you would run it so I can avoid changing this to needs review before I catch any further problems?
I ran it through coder and besides some camel case errors I can't avoid because I'm calling a 3rd party library I think I've fixed most errors. I'm not sure if I just hadn't updated git with the latest but I didn't find most of the errors listed above. The ones I did find I went and fixed though.
What can I run to make sure I've gotten everything?
Thanks,
James
Comment #11
Barnettech CreditAttribution: Barnettech commentedI've gotten rid of all problems that I know of. Please excuse that this is my first module submission and am doing the best I can to satisfy the requirements. If there is a way for me to run your automated tool to check for problems let me know. I ran things through coder and besides some camel casing which I cannot avoid since I'm calling google's api (which uses camel casing) I believe I've fixed all the problems as requested. I do appreciate you folks guiding me through this process and I'm eager to succeed here. I actually have quite a few other modules I've written here at Babson which I'd like to contribute, which I think others would greatly benefit from. Many of them integrate very nicely with Acquia Commons for any who are interested. We are really looking to galvanize the Drupal education space.
thanks,
James Barnett
Architect Babson College
jbarnett@babson.edu
Comment #12
patrickd CreditAttribution: patrickd commentedReview of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #13
Barnettech CreditAttribution: Barnettech commentedOk now we're in business. I was able to run PAReview myself: All errors have been treated. The google_tests.php file is a command line test script hence the closing ?> php tag, so that is not in fact an error. As far as the README.txt file not having lines exceeding 80 characters I'd like to put my foot down and say it makes my README file less readable and seems ridiculous. Put please let me know if this serves some greater purpose. I saw doItDaves post about how 50% of folks find a lot of these rules as an impediment -- I'm trying to learn from this process and not pass judgement just yet. But just to write a readme file I had to write a shell script to find which lines of my README.txt exceeded 80 characters:
Here's the script
I'm trying not to have an opinion yet but this seems ridiculous especially since I can barely finish 1 or 2 well polished sentences without going over 80 characters.
Review of the 6.x-2.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
I'm hoping to get DROOGLE promoted today! Let me know what else I can do. I ideally would like to help with the Drupal community though my free time is tough to come by these days, so please take my comments as having the correct spirit of promoting progress, success and cooperation.
Thanks,
Best,
James Barnett
Architect Babson College
jbarnett@babson.edu
Comment #14
Barnettech CreditAttribution: Barnettech commentedSee my comment above and the pareview results with my comments. thanks for the review.
Comment #15
jthorson CreditAttribution: jthorson commentedbarnettech,
The 80 character limit is a recommendation to help afford readability and prevent needing to scroll across long lines for folks who are installing your module via an ssh window or the command-line, as many server admins do not install the GUI interface.
For an example of how the readability is affected by long lines, check out the current version of your README.txt file (which is not line-wrapping at 80 characters) at http://drupalcode.org/sandbox/barnettech/1327544.git/blob/refs/heads/6.x...
Other comments:
Comment #16
Barnettech CreditAttribution: Barnettech commentedThank you for your code review.
I've fixed 1, 2, and 4. I ran my module through the coder module and it's not showing any errors for your #3 above. I was very impressed with your review and loved the comments regarding variable_get and system_settings_form($form), and I greatly appreciate the help in having a 2nd set of eyes make sure my code is not vulnerable. These items have been fixed. So we're left with #3. I've already jumped through 100s of hoops for the coder module and fixed up my syntax to comply with drupal.org The coder module already had me fix my spacing regarding "{}" and "()" characters. The coder module is now satisfied with all of my syntax, spacing, etc. There are a few camel casings left because I'm calling google's api and they use camel casing differently. google_tests.php is a command line test so has a closign ?> php tag.
I believe the right thing to recommend is ... if you want different spacing being submitted you should put a patch in for the coder module.
I believe I've satisfied all requirements now?
I'm learning quite a bit from this process, and hope to finally get my project approved this round.
Cheers,
James
Comment #17
jthorson CreditAttribution: jthorson commentedCoding Standards
I'm glad your module is passing the Coder reviews ... unfortunately, the standard we review against is http://drupal.org/coding-standards and not the Coder module. The first line of the standard on that page reads "Use an indent of 2 spaces, with no tabs." As this is not reflected in your code, the only logical conclusion a I can come to as a reviewer is that noone has yet directed you to the Coding Standards page, which is *strongly recommended* reading for all contributors. Please have a look through the content on that page, and ensure that your code aligns with the established Drupal standards.
I realize that not all published modules are fully compliant ... the standards change and evolve over time, and contributors subsequent modules are not subject to any peer review. That said, one of the primary goals of this review process is to ensure that new contributors are aware of (and willing to abide by) these standards ... the indenting and control structure spacing included in your module suggests that you have not yet been introduced to them.
Repository Structure and Branch Naming
Please review http://drupal.org/node/1015226 for information on tag naming. Your branches are fine, but contrib module tags should follow the 6.x-1.0-beta1 convention, as opposed to 6.x-beta1 (which is the pattern for Drupal core). Also, I'm not certain why you have seperate 6.x-1.x and 6.x-2.x branches at this point; but I suspect it's not necessary. A 2.x branch would be used to indicate a rewrite or a break in backwards compatibility with previous releases, or in some cases the addition of major new feature ... though this final case can still be handled within a point release (when going from 6.x-1.1 to 6.x-1.2) assuming it's still backwards compatible with previous versions.
Finally, I noticed that you have recent commits against both the 6.x-1.x and 6.x-2.x branches, but not all of your commits were applied to both branches of code. Again, I'm not sure if this is intentional or not, but it may result in some of your fixes only being applied against one code stream; and a reviewer who is looking at the other code stream is likely to flag the exact same issue all over again. Generally speaking, my advice for contributors is to stick with a single branch throughout the review process, just to make sure that all of your fixes get included in the final code when it becomes time to promote the project and create an official release ... in your case, I suspect that some of these have been missed (in which case, I'd consider performing a 'git merge' to bring the branches back together again and ensure that all of the fixes are included).
Comment #18
Barnettech CreditAttribution: Barnettech commentedIs there any automated tool I can use to highlight my spacing / syntax infractions so I don't miss any? I will clean up the repository as well of course. I am switching from svn to git ... Hence i was a bit clumsy but the latest branch is up to date.
Comment #19
Barnettech CreditAttribution: Barnettech commentedOn the page you referred me to http://drupal.org/node/1015226 it says
So don't say I didn't read on how to work with drupal standards.
Perhaps it is because this is a Drupal 6 module jthorson and you are applying Drupal 7 standards which may have changed?
This is the Drupal 6 version of Droogle. When I submit the module for Drupal 7 I promise to use the Drupal 7 coder module for code standards compliance.
The page you referred me to as I say above confirms I have complied with all code standards for submitting a DRUPAL 6 module.
Comment #20
jthorson CreditAttribution: jthorson commentedI'd be surprised if Coder isn't flagging the spacing and control operator spacing items - make sure you've got it configured to run on 'minor'. For a really critical review, you could also add the Coder Tough Love plugin to Coder - I've yet to see a project that can pass that 100% ... if you can pass the CTL review, you can pass anything. ;)
Other tool options include the PAReview.sh script or Drupal Code Sniffer ... though I don't know for certain whether Code Sniffer flags the indentation or not.
Comment #21
jthorson CreditAttribution: jthorson commentedJust so anyone reading isn't confused, you were referring to the coding standards link, but posted the branch naming link ... the link in the last comment should have been http://drupal.org/coding-standards
I didn't say that ... I said that the spacing and control structures within your code suggest that you aren't aware of the standards. The alternative is that you *are* aware of the standards, and blatantly choose to disregard them, in which case any time I spend reviewing your application is wasted time. For your sake, I chose the first interpretation.
I don't see where that page confirms anything, but your code repository confirms that you have not.
LINE 3: Missing @file declaration
LINE 12. Incorrect Function Documentation Docblock structure
LINE 18: Improper Indentation
LINE 18: Extraneous permission
Why declare a 'use google apps' permission that you never use?
LINE 30: Improper use of t().
Translators don't necessarily understand code structures such as <<<, especially when normal quotes would suffice.
LINE 38: Missing function declaration docblock
Explanation as per that for line 12, above.
LINE 40: Redundant Code
should be
LINE 66: Wrong default argument for variable_get()
$allowed = variable_get('droogle_upload', '');
If you're expecting $allowed to be an array (using
array_values($allowed)
later on), your default value should be an array.Conclusion
Note ... I'm not even 100 lines into your module. Trust me when I tell you that Coder will *not* catch everything ... it is simply a tool which will help you catch some of the more common issues that are easy to identify through automation.
Also, please keep in mind that those performing reviews are volunteers, giving up their own free time to try and help you through this review process. Getting frustrated, suggesting we don't know what we're talking about, or trying to push toward premature approval of your application is not productive; nor is it conducive to encouraging the reviewer to perform a follow-up review.
Second Opinions & Complaints
If you have an issue with a review or one of the coding standards you're being asked to abide by, then feel free to come to the project applications group page at http://groups.drupal.org/code-review and request a second opinion or file a complaint there.
Comment #22
Barnettech CreditAttribution: Barnettech commentedI do appreciate your volunteer efforts, I am volunteering to submit this code and all my code in hopes it greatly improves the Drupal education space. The only thing I'm rushing to do is to get the project cleared so I can work on more exciting features and new modules to benefit my employer and others in the community. Now that we are down to spacing and line breaks in doxygen conversation I'm happy that is the only thing left to figure out. I mean no disrespect, I just want to submit this code. I'll seek council from other friends in the boston drupal meetup on how to proceed. I do sincerely thank you and others in this thread for helping in making droogle a better product.
James
Comment #23
Barnettech CreditAttribution: Barnettech commentedI am working on my DOXYGEN documentation which does need work and is the only thing I know needs doing prior to release.
Comment #24
Barnettech CreditAttribution: Barnettech commentedDOXYGEN documentation was added for all files and functions. I cleaned up the naming of my tags. Thank you all for your help in reviewing my first module. As we run toward progress we try not to forget the important details, less we leave our shoes untied and trip so to speak. I do sincerely applaud your efforts.
I reviewed my module and compared the errors in the coding module to other core contributors that I greatly respect -- Moshe Weitzman, MerlinOfChaos, both of them have contributed enormously to drupal. The droogle module now complies to coding standards more than the Migrate module (Moshe) and Chaos Tools module (Merlin).
I am aiming to get an A here, and I certainly think these 2 are respected members of the community -- the coder module is throwing fewer errors to check with droogle than their modules. I am contributing this module because it is good to give back to the community, but I only have so much time to give. These are certainly not billable hours. I'm going to ask a colleague to review my code for me. My colleagues have a great desire to make sure our code is secure and top notch, and I strive to have our team contribute more to the review process, and to contribute as much as we can to the drupal community. I did not have them do it previously because I wanted to just get a feel for the process around here.
I hope you will respect their decision to either set this as "RTBC" or to set this module to "needs review".
I am requesting to have this 3rd party look at this for a final review.
Again I really do appreciate everyone who's contributed another set of eyes to insure the quality of this code.
Comment #25
jthorson CreditAttribution: jthorson commentedAnyone is welcome to review an application, and anyone other than the applicant themself is welcome to set it to RTBC. The only request is that your reviewer familiarize themselves with the contents of the Review full project applications documentation, and all referenced standards and security practice pages. This will help ensure that your RTBC status doesn't get bumped back down to 'needs work' under final review by a Git administrator.
On another note, you have not addressed at least five of the items flagged in my last comment, only one of which is a 'spacing or line break' issue. Please ensure that you have fixed in code (or addressed in comments) each of the flagged items before re-setting the application to 'needs review'.
As per your comments regarding other contributors and their modules ... I can appreciate that it seems like a double standard; but comparing your module to other modules which also do not abide by the standards does not change the fact that the standards exist, and are the measure used in this application process. The fact is that a successful application means you could publish other modules without any sort of peer review or abiding by ANY coding standards (not that you should!), which is one reason that we place as much focus as we do on ensuring that you are aware and willing to abide by the standards within *this* application process. Frankly, if you aren't willing to do it here, there is nothing to prevent *your* future modules becoming the one someone tries to use as an example to unsuccessfully justify their own non-compliant code.
Comment #26
Barnettech CreditAttribution: Barnettech commentedSo I just put the last of the fixes in that you requested jthorson. Again I am learning a lot from this process and actually kind of enjoy it, but always feel pressed for time to move on to my next task. So you are saying this is a one time process and once I have the right to publish modules I wont have this type of review (for better or worse) again? Is this how others are publishing with far more errors than this module has?
If you will be patient with me, I'm willing. What else do I need to fix / learn to submit modules? I'm only frustrated because I like to get a lot done, which usually leads to success, but sometimes it is worth while to look at each detail, I do appreciate that. And it was / is frustrating that others I respect are submitting modules with far more compliance issues.
But I will see this through with an open mind eager to learn and succeed with this process, now and for future modules. What do I need to fix next?
Comment #27
Barnettech CreditAttribution: Barnettech commentedJust so you know I'm fairly pathetic here refreshing this page waiting for the next thing to learn / fix. Let me know what else is waiting for me to learn / fix -- the module is working great, but I'm eager to gain acceptance by this group and learn your processes.
Comment #28
jthorson CreditAttribution: jthorson commentedI don't have time to perform another review tonight ... though I will point out that the default value for variable_get() on your $allowed command should be array() ... I'm not sure if array_values(NULL) throws a PHP warning or not, but since the variable is expected to be an array, an empty array makes a better default value than NULL.
Anecdotally speaking, until very recently, most reviews would go anywhere from a few days to four weeks between follow-ups ... I only mention this to put things in perspective, and as encouragement for you to stop clicking 'refresh'. ;)
Comment #29
Barnettech CreditAttribution: Barnettech commentedOk I'll stop hitting refresh then :) boy that was a long few hours ... just kidding. Have a good night. Thanks for the perspective... I had heard you really need to have a co-worker do the review, or someone from a local drupal meetup, but I feel like we're getting there. After I learn the process I can teach my team.
Cheers,
James
Comment #30
jthorson CreditAttribution: jthorson commentedUp until about a couple of months ago, the 'needs review' queue was in the neighborhood of 400 applications long ... so if you wanted a quick approval, that was largely the case. The addition of the automated review scripts, along with some relentless reviewing by a select few helped clear the entire queue ... and with luck, we'll never let it get back to that state again. :)
Comment #31
Barnettech CreditAttribution: Barnettech commentedKudos to you all, your efforts have helped with drupals growth and it's become very fun to work with, it's become so easy to impress clients / employers
Comment #32
Barnettech CreditAttribution: Barnettech commentedOk the line now reads
Anything else that needs to be fixed?
Comment #33
jthorson CreditAttribution: jthorson commentedI had answered this question, but then lost my form submission ... and just now realized that I didn't re-answer when I re-typed my reply ... but your statement is correct that this is a one-time process, which is a bit part of the reason that we're so sticky about the seemingly 'little things'.
1. Question: Is there any particular reason why, after all this, your module still only uses a single space for each indent, instead of the recommended two?
2. Docblocks are much better, but there are still some inconsistencies. Use 'Implements' instead of 'Implement', ensure your capitalization is consistent with the exampls provided in the standard, remove blank lines above and below an 'Implements hook_x().' declaration (and make sure you include blank lines where the standard asks for them), and add a period at the end of each declaration and description. Ensuring these patterns are followed allows your module to be parsed by Drupal's Doxygen parser, so that help pages for your code can be auto-generated on sites such as api.drupal.org or drupalcontrib.org.
3. Overall spacing is inconsistent ... there are a few spots where you have two empty blank lines between the code and a closing '}', which affects the readability of the code.
4. For readability, inline code comments (lines using '//') should have a space between the '//' and the comment/code.
5.
$content = 'got to uploadDocumemt';
This looks like a 'debugging' leftover to me ... all debugging related code should be stripped out of the module before applying.Really ... I can only stress that you need to make sure your code abides by all of the standards listed at the coding standards page and all pages it links to before setting things back to 'needs review' ... The goal here is to help you determine what items need to be checked before promoting a module to 'full project' status, not for us to provide you a line-by-line summary.
Comment #34
Barnettech CreditAttribution: Barnettech commentedMy response is as I've said, I did an audit of many other Drupal modules, many of the authors being leaders in the community and none of them even passed the coder module's standards. So I figured that eventually someone would flip this project to "RTBC". This module does pass the coder module test and Pareview.sh test btw (if anyone else is reading this). I also don't know you at all, don't know your standing in the community and did not know if you were in fact wrong, and the coder module was right ... that my module already complied with coding standards due to it's having passed the coder module automated test.
I will be going through my code today with a fine tooth comb looking to comply with the coding standards page. Again thank you for you time and energy in helping me get this module approved. I know there is a sore lack of folks helping with core bugs, and the module approval process, and I'm not looking to disrespect your helpfulness, I'm just new to the process and doing the best that I can to learn, to be helpful, and to contribute. Those are my only intentions -- and I am sure those are your intentions as well. So again .... thank you despite my many questions. I'm definitely the guy who questions "authority" but that is also how you learn rather than blindly following folks off a cliff.
Cheers,
James
Comment #35
Barnettech CreditAttribution: Barnettech commentedI went through the code and addressed all the items requested above in comment #33. I also ran the coder module through looking for minor errors and fixed all of those.
I fixed up the Droxygen especially looking for spacing, punctuation errors, etc.
I fixed it so the spacing for all lines in general used the 2 space indentation.
I put a space after comments (lines starting with //)
I looked at all the lines with a "}" character and it seems ok now.
I read through all of the coding standards page: http://drupal.org/coding-standards#indenting
Comment #36
Barnettech CreditAttribution: Barnettech commentedJust posted this comment about the review process: http://groups.drupal.org/node/195303#comment-645748 It's all very interesting to see how this works :)
Comment #37
Barnettech CreditAttribution: Barnettech commentedBeen doing my own code review trying to carefully compare the coding standards page to my code. I found some variables today which did not conform and I changed them to lower case separated by underscores versus the camel casing I was using which conformed to Google's coding standard. I see no other things that do not conform to drupal standard.
Now in minor mode on the coder module, there are no errors whatsoever (although some camel casing remains but that is the only way to call the google api -- so must stay, and coder incorrectly says I'm missing some check plain statements which is not the case actually).
Comment #38
Barnettech CreditAttribution: Barnettech commentedJust started helping with the review process myself of another soul in the review process: http://drupal.org/node/1166144#comment-5347266
Comment #39
Barnettech CreditAttribution: Barnettech commentedhttp://drupal.org/node/1166144#comment-5347326 I started helping with the que with what I've learned thus far
Comment #40
Barnettech CreditAttribution: Barnettech commentedJust commited to my branch after running my code through coder tough love.
Droogle code now passes coder tough love and minor errors checks in coder module, and I manually searched for infractions to drupal.org coding standards
I'm hoping if someone has the time to review this again, there wont be any infractions they could find. Fingers are crossed. I'll continue to help others in their review process as I wait.
James
Comment #41
jthorson CreditAttribution: jthorson commentedIf you keep posting to your own thread like this, it's going to look to reviewers like the thread is busy, which indicates that someone else is already actively discussing the application with you ... at which point they simply skip over and move on to the next applicant.
Also ... it's considered rude to send unsolicited emails to your reviewers ... if anything, it's going to ensure that they do *not* choose your module the next time they have a few minutes to spare for another review. I've suggested before that you simply need to have a bit of patience, and accept that this process takes time ... and count yourself fortunate that it's not taking four weeks between followups, like it would have had you applied back in March.
Comment #42
klausiThere are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-2.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.
manual review:
Comment #43
Barnettech CreditAttribution: Barnettech commentedI cleaned up the master branch, just README.txt is there now. Also I moved everything to 6.x-1.x since there was no need for 6.x-2.x yet, since there is no major release.
Another person reviewed my module at the Boston Drupal Meetup and I fixed some items in response to that review. Google passwords are encrypted in the database, the Droogle OG settings on the settings page show conditionally only if OG is even installed.
Klausi: I fixed up the drupal_set_message to use the t( function appropriately, thank you.
Klausi: As I've posted google_tests.php is a file to run at the command line as described in the README.txt file and so needs the closing ?> php tag
Klausi: At the Drupal meetup in Boston we were cleaning up my repository and it seems to me the repository was a bit messed up after that session, I've now fixed it up and the sandbox should now be showing the most up to date code in 6.x-1.x Sorry for this confusion.
Klausi: I've run coder in minor mode (using the DRUPAL 6) module and there are absolutely no errors showing. I previously ran pareview on my code and there were no errors. Now I run it and there are many, but many are applying Drupal 7 and Drupal 8 coding standards and don't apply to a Drupal 6 module and in fact I'm confused if a Drupal 6 module should comply with Drupal 6 standards or Drupal 7 standards -- my guess is it depends on the reviewer, so I could end up in an endless loop fixing things to Drupal 6 standards for one reviewer and Drupal 7 standards for another? Here is one such suggestion from pareview which is a change in Drupal 7 coding standards: 44 | ERROR | Missing parameter type at position 1 In Drupal 7 you would do: @param bool $flagged but in drupal 6 you were required to do: @param $flagged
Most of the errors in pareview are spacing errors and coder says there are no errors in this area. Because coder in minor mode is geared to Drupal 6 and pareview is setup to review Drupal 7 and Drupal 8 modules I am leaving the rest of my code unchanged, since coder and coder tough love in Drupal 6 say there are absolutely no further problems in minor mode. I also see you ran drupal code sniffer but that is also a Drupal 7 tool (http://drupal.org/project/drupalcs). When I upgrade this module to Drupal 7 I will be sure to use these new tools.
Let me know what else I need to do, and I will willingly listen and comply (even if your thoughts conflict with my thoughts written here). I will be submitting a Drupal 7 version of this module within the next 6 months or so btw. Right now we are using Acquia's distribution "Drupal Commons" (https://network.acquia.com/downloads/drupal-commons) and this distribution is still on Drupal 6. My group at Babson will be assisting Acquia in upgrading this distribution to Drupal 7 shortly and I'll upgrade Droogle at that time to Drupal 7 as well.
Thank you for your review.
Thanks,
Barnettech
Comment #44
klausiFrom https://drupal.org/coding-standards
Your module is new code, so you should follow the new standards.
Comment #45
Barnettech CreditAttribution: Barnettech commentedOk, no problem.
Comment #46
Barnettech CreditAttribution: Barnettech commentedI just spent a few hours cleaning this up. There are no longer any errors on Pareview. Let me know what else needs doing. thanks!
Comment #47
klausiReview of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.
manual review:
As you might know I started #1410826: [META] Review bonus, so feel free to list any reviews in the issue summary that you have done and add the tag, after you fixed all issues in your own module.
Comment #48
Barnettech CreditAttribution: Barnettech commentedComment above (http://drupal.org/node/1328366#comment-5308244) encouraged me to use this function, so I did. I actually did not know about it and finds it saves a few lines of code here. Then I want to do some special handling of passwords, so still use function droogle_settings_form_submit($form, &$form_state) for this.
Yes, and I chose not to, the separate module would only be about 20 lines of code with hook_menu and then a callback which would just call the api function. Thanks for the suggestion.
A lot of these API functions were made to be as generic as possible for use from the command line without the need to bootstrap drupal.
pareview now reports no errors, all items mentioned above have been fixed. Thank you for your review Klausi. I am in the middle of reviewing another applicants module but it is fairly involved and is taking a while: http://drupal.org/node/1379794, although I already ran pareview on his project and did a quick manual review of the code (I did leave one comment for him you'll see). I'll install the project and will then do another manual review for him. Regardless if my module gets approved I will do that review of course. I am looking to get more involved in the community.
Comment #49
Barnettech CreditAttribution: Barnettech commentedAdding PAReview: review bonus tag, I've done quite a few rounds of reviews
Comment #50
klausiDon't forget to set the status to "needs review" if you have completed your improvements.
Comment #51
Barnettech CreditAttribution: Barnettech commentedI have completed the improvements thank you, when I put in the tag it defaulted back to needs work, thanks Klausi
Comment #52
klausit('<p>Droogle does for Drupal what BBoogle does for Blackboard. We integrate Drupal with google apps and other Google applications.</p>')
": The p tags should be outside of the t() function call.I'm removing the review bonus tag, you can do some more reviews and assign it back to you then if you want.
Comment #53
Barnettech CreditAttribution: Barnettech commentedAll items in #52 above have been fixed.
the answer is yes, no files can be uploaded to google docs without this being set.
Comment #53.0
Barnettech CreditAttribution: Barnettech commenteddetailed some of the reviews I've done while waiting for my project to be reviewed.
Comment #53.1
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #53.2
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #54
Barnettech CreditAttribution: Barnettech commentedJust finished doing 3 reviews, so adding the PAReview: review bonus tag.
Comment #54.0
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #55
klausiReview of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.
manual review:
Comment #55.0
klausiUpdated issue summary.
Comment #56
Barnettech CreditAttribution: Barnettech commentedThank you Klausi, all has been fixed. I've actually been doing reviews for the last 2 days, and got a bit obsessed with it, so added some of the links above and am flipping it back to have the PAReview: review bonus tag. I wonder if you guys could run a git post commit hook to run pareview and auto post to the thread if there are errors? It would save you a lot of time having to run it manually. You probably are already thinking along those lines.
Comment #56.0
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #57
themebrain CreditAttribution: themebrain commentedReview of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Manual review:
- The sequence strings in translate file must be matched with the string in t function, so here be careful with break line and multiple spaces. The same in other places. For example:
drupal_set_message(t('the Droogle og password was not entered, every time this form is saved all passwords must be re-entered, if not
using Droogles og feature please ignore this message'));
- Please removed the debuging code before submit such as:
// print_r($feed);
- A lot of inline comments using /* */, it seem different from the comment formatting conventions http://drupal.org/node/1354
- Some variables don't match with the naming convention http://drupal.org/coding-standards#naming, for example:
$Entry = '';
$Url = "https://docs.google.com//feeds/default/private/full/folder%3A" . $collection_id . "/contents";
Comment #58
Barnettech CreditAttribution: Barnettech commentedI fixed the all items mentioned in comment #57 although in this comment:
I looked at your example and the code works and as well conforms to the drupal.org style guide
Let me know what else needs fixing and what else I may have missed and I will diligently fix it. Thank you.
Comment #59
themebrain CreditAttribution: themebrain commentedIt looks good now
Comment #60
klausiComment #61
Barnettech CreditAttribution: Barnettech commentedEverything was addressed in comment #60 above. Thank you for the review.
Comment #61.0
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #61.1
Barnettech CreditAttribution: Barnettech commentedUpdated issue summary.
Comment #62
Barnettech CreditAttribution: Barnettech commentedAdding Pareview bonus tag back after having done some more reviews.
Comment #63
klausimanual review:
But that it just a minor issue, otherwise I think this is RTBC. Thank you for your patience and your endurance during this process! Thanks also for helping out in other applications.
Comment #64
klausiRemoving review bonus tag.
Comment #65
zzolo CreditAttribution: zzolo commentedThanks for your contribution, @barnettech! Welcome to the community of project contributors on drupal.org.
I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.
Thanks to the dedicated reviewer(s) as well.
Comment #66
Barnettech CreditAttribution: Barnettech commentedThanks for all who helped with my review. I'm excited to take this step toward getting more active in this great community.
Comment #67.0
(not verified) CreditAttribution: commentedUpdated issue summary.