guillaumev [guillaumev]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

We just review a module/theme per applicant; let us know which one you want to be reviewed.

Thank you for your reply.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3. I am not sure what the purpose of changing the global language before the validation handlers have been executed would be. Do you have a use case for the proposed module?
4. The module seems too simple to be used for a CVS application.

This looks solid, let's get guillaumev an account!

Hi guys,

Just to recommend Guillaume's work in general in case that would make you more comfortable.

* Guillaume is a French software engineer and has been working on Drupal modules and implementations for the last 9 months in a corporate environment inside a company (BeezNest) that is spearheading the Drupal professional development & community in Peru (hosting most of Drupal Peru's events, reportedly the most active local group at a global scale, which means at least two events a month, of which one code sprint).
* He's been participating on drupal.org for a while.
* He's a certified Zend Engineer.
* He's coached richie06031983 to contribute to Spanish translations.
* He's already contributed several blog articles about distinct aspects of Drupal: see http://beeznest.wordpress.com/author/gviguier/
* He's very respectful of rules.
* He's working in an environment with two other Zend Engineers, of which at least myself (responsible for the Chamilo module and previously Dokeos module).

In all aspects, he's been an incredible technical asset and I think it would simply be a bad idea not to allow him a more direct access to adding code to Drupal modules.

This, of course, doesn't say much about the fact that his modules respect the Drupal rules or not, but that's for somebody else to judge.

Yannick

I just had a look to the code, and it looks fine, so moving back to RTBC

BTW alex_b is suggesting to RTBC this and I think ywarnier had a status change by error(looking at the comment post times).

I have heard about guillaumev a year ago and how interested was him on contributing back to community, now he is coming with a working module ready for a beta release!

So a plus for him because of his persistance, it looks like he will be a responsible maintainer!

1. See http://drupal.org/coding-standards to understand how a module should be written; in particular, see how functions should be named.

2.     drupal_set_message(t('An error occured while calling the Agrovoc API'), 'error');
       watchdog('agrovoc_api', t('The Agrovoc API returned the following response').': '.$response, NULL, WATCHDOG_ERROR);
   

   The second parameter for watchdog() is not correct.

3.   if ($vocabulary->agrovoc && user_access('administer taxonomy')) {
       return TRUE;
     }
     else {
       return FALSE;
     }
   

   The code can be re-written as

     return ($vocabulary->agrovoc && user_access('administer taxonomy'));
   

4.       $form['settings']['agrovoc'] = array(
           '#type' => 'checkbox',
           '#title' => 'Agrovoc vocabulary',
           '#default_value' => $vocabulary->agrovoc,
           '#description' => 'Whether this vocabulary should allow terms from the Agrovoc thesaurus to be added'
         );
   

   Strings shown in the user interface should be translated; form field descriptions should have a final period.

5.   $sql = "SELECT vid FROM {agrovoc_taxonomy} WHERE agrovoc_id = '%d' AND agrovoc_lang = '%s'";
   

   The placeholder for integers doesn't need to be wrapped in '; if the parameter is really a string, then the placeholder should be '%s'.

6.   while ($vocabulary = db_fetch_object($result)) {
       if ($vocabulary->vid == $vocabulary_id) {
         $is_in_vocabulary = TRUE;
       }
     }
   

   The loop should break when it finds the vocabulary ID; it doesn't make sense to fetch data from the database when the vocabulary has been already found.

The coding standards report that module functions should not contain upper case letters.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

I am giving credits to the users who participated in this issue.