morningtime [morningtime]

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

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions. The reported feature is already implemented in http://drupal.org/project/gigyaToolbar.

The project page for the existing project reports the following description:

Share – One click share to over 80 social destinations.

I took it was referring to the same feature as reported by you.

Hi @morningtime, thanks for the application. Please be patient with this process. It is done by volunteers and as a community we like to ensure that this process remains valuable to everyone, hence why there has been lots of questions about how your module compares to others. If you have opinions on this subject, please read and comment #703116: Our CVS account application requirements are obtuse and discourage contributions

Overall, I would like to see only one gigya module. There is no reason to make multiple modules that integrate with the same service. I hope that if/when you get CVS access that you will work towards this goal.

The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

1. You should have a basic README.txt describing your module and how to install it.
2. You should have a basic implementation of hook_help, if not more.
3. You create variables put do not remove them in an uninstall hook.
4. Please follow Drupal coding standards. This includes things like spacing. It also includes more than just PHP files. http://drupal.org/coding-standards

5. 
     $items['admin/settings/gigya_share'] = array(
       'title'            => t('Gigya Share'),
       'description'      => t('Settings for your <a href="http://www.gigya.com/site/partners/api.aspx#&&history=Bookmarks" target="blank">Gigya Share</a> Share/Save buttons.'),
       'page callback'    => 'drupal_get_form',
       'page arguments'   => array('gigya_share_admin_settings'),
       'access arguments' => array('administer gigya_share'),
       'file'             => 'gigya_share.admin.inc',
     );
   

   Menu title and description do not need to be run through t() as this is the default behavior. Also, you are using t() poorly here for the description. Please see the documentation for t().

6. Ensure you are using drupal_urlencode instead of rawurlencode, http://api.drupal.org/api/function/drupal_urlencode/6

7.   $form['gigya_share_general_settings'] = array(
       '#type'          => 'fieldset',
       '#title'         => t('General'),
     );
   

   There is no point in putting the only elements in a form into a fieldset.

8. // Only show comments on the full non-teasered page
   

   This should be an administrative option.

9. _gigya_share_create_button should involve some sort of theme function as it it providing output.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

To reply to comment #8, I don't see any differences between the widget implemented by this module, and the widget implemented by the existing project.
As reported in comment #7, the existing project includes already the same functionality present in the proposed module.

After looking at both modules, this one simply puts a button to share in the links of a node or in the content (similar to ShareThis), while the other implements a full toolbar (and is poorly written, might I add) as can be seen: http://www.channelone.com/

Apparently there already is a gigya module: http://drupal.org/project/gigya
This seems to be a fairly robust module integrating with gigya, though it does not seem (from the project page description) to be providing a ShareThis functionality. In a perfect world, @morningtime would talk with the maintainer and put this functionality in that module. Like I said above, there is no reason to have multiple modules that integrate with the same service.

@morningtime, can you please update with your thoughts.

There have not been replies in more than a week. I am marking this report as won't fix.