[D7] Simple Weather

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxlarsdesigns2203553git

I'm a robot and this is an automated message from Project Applications Scraper.

Generally speaking I don't like seeing SASS inside of modules, there is really only one line of css, you can remove the SASS, and config.rb and just keep the css file with the single line of css.

It seems to be a bit confusing atm on which you need US Zip Code or WOEID. It should be obvious to use Zip Code for US and WOEID for everywhere else.

You should use the libraries module API, rather then just file_exists /sites/all/libraries/. And you should add libraries to the dependencies.

Besides that it seems to work fine for me.

Alright it looks fine by me.

There are still some errors http://pareview.sh/pareview/httpgitdrupalorgsandboxlarsdesigns2203553git

It appears you are working in the "7.x-1.0" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

* 7.x-1.0
  remotes/origin/7.x-1.0
  remotes/origin/HEAD -> origin/7.x-1.0

Review of the 7.x-1.0 branch:

• Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/simple_weather.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 7 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
    54 | WARNING | #options values usually have to run through t() for
       |         | translation
    55 | WARNING | #options values usually have to run through t() for
       |         | translation
    70 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
    71 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
    96 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
    97 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   201 | WARNING | Unused variable $options.
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

FILE: /var/www/drupal-7-pareview/pareview_temp/js/simple_weather.js
--------------------------------------------------------------------------------
FOUND 32 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
 18 | ERROR | Expected 1 space before "+"; 0 found
 18 | ERROR | Expected 1 space after "+"; 0 found
 18 | ERROR | Expected 1 space before "+"; 0 found
 18 | ERROR | Expected 1 space after "+"; 0 found
 18 | ERROR | Expected 1 space before "+"; 0 found
 18 | ERROR | Expected 1 space after "+"; 0 found
 18 | ERROR | Expected 1 space before "+"; 0 found
 18 | ERROR | Expected 1 space after "+"; 0 found
 19 | ERROR | Expected 1 space before "+"; 0 found
 19 | ERROR | Expected 1 space after "+"; 0 found
 19 | ERROR | Expected 1 space before "+"; 0 found
 19 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 20 | ERROR | Expected 1 space before "+"; 0 found
 20 | ERROR | Expected 1 space after "+"; 0 found
 21 | ERROR | Expected 1 space before "+"; 0 found
 21 | ERROR | Expected 1 space after "+"; 0 found
 21 | ERROR | Expected 1 space before "+"; 0 found
 21 | ERROR | Expected 1 space after "+"; 0 found
 25 | ERROR | Expected 1 space before "+"; 0 found
 25 | ERROR | Expected 1 space after "+"; 0 found
 25 | ERROR | Expected 1 space before "+"; 0 found
 25 | ERROR | Expected 1 space after "+"; 0 found
--------------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/simple_weather.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 10 WARNING(S) AFFECTING 12 LINE(S)
--------------------------------------------------------------------------------
  12 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
  25 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
 115 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
 126 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
 131 | WARNING | Line exceeds 80 characters; contains 82 characters
 158 | ERROR   | Inline comments must start with a capital letter
 175 | WARNING | Translatable strings must not begin or end with white spaces,
     |         | use placeholders with t() for variables
 176 | WARNING | Only string literals should be passed to t() where possible
 180 | WARNING | Empty return statement not required here
 184 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
 197 | ERROR   | The $text argument to l() should be enclosed within t() so
     |         | that it is translatable
 202 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

Source: http://pareview.sh/ - PAReview.sh online service

Manual review:

• You should use 7.x-1.x branch in git instead of 7.x-1.x-dev or 7.x-1.0.
• simple_weather.info - package uses to combine several modules into one group. Your module consists of only one module so package = Simple Weather is excessive.
• In hook implements addition documentation comments are unnecessary. Use just Implements hook_foor_bar(). See https://drupal.org/coding-standards/docs#hookimpl.
• simple_weather.module - simple_weather_woeid_anchor() function is only using in simple_weather_block_configure() function. It's better to place body of simple_weather_woeid_anchor() functinon inside simple_weather_block_configure().
• I think instead of two functions simple_weather_zip_code_valid($element, &$form_state) and simple_weather_woeid_valid($element, &$form_state) you shoud use only one because they have a similar functionality.
• simple_weather_content() - you shoud add a doc comment for return value.
• For variable_get() function you shoud provide a default value.
• "Only a ZIP code or WOEID can be submitted." but you added both values to Drupal JavaScript settings object in simple_weather_content().
• $content = '<div id="simple-weather"></div>';. All HTML code should be added via theme() function https://api.drupal.org/api/drupal/includes!theme.inc/function/theme/7.
• You should use Libraries API module for adding external libraries https://drupal.org/project/libraries.
• You shouldn't split t() function. Use replacements:

  <li>$woeid_lookup = t('You can use the !woeid_link lookup tool to find the unique WOEID for your location. Works world wide!', array('!woeid_link' => $woeid_link))

I think adding select list with "USA ZIP Code" and "Where On Earth Identifiers (WOEID)" values is better for UI. You can hide unselected value depend of #states. Seу https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h... and Example module https://drupal.org/project/examples. This will do code smaller.

Thanks for contribution anyway!

Have you pushed all chenges to git repo? I can't see only 3 changes.

You should put $content = '<div id="simple-weather"></div>'; inside theme_weather_output(). In this case other people will be able to overwrite it in custom theme.

Use replacements inside this t() too:

    $msg = t('The file jquery.simpleWeather.min.js is missing and is required by
              the Simple Weather module. Download the') . " $download_link " .
              t('jQuery plugin and read the README.txt file found inside the
                simple_weather.module file for installation instructions.');

The Libraries API module is recomended way for adding external libraries to Drupal.
This code DRUPAL_ROOT . '/sites/all/libraries/simpleWeather/jquery.simpleWeather.min.js' will not work for multisite (/sites/example.com/libraries/).

See the words from Project application review administrator https://drupal.org/comment/8546253#comment-8546253.

• You should probably update the git clone command in initial post to point to the updated 7.x-1.x branch.
• Move code using the default jQuery $(document).ready() trigger to use Drupal.behaviors instead (documentation is available over here). In some cases having a mix of the two can lead to some odd interactions.
• Is there a way to move the HTML that is inserted through JS to a place where it could be provided as a default but overridden in a site-specific instance if needed? Right now if I wanted to use this on a site but change the order or type of the HTML elements I'd have to hack/replace js/simple_weather.js which blocks automated updates after that (through the update manager or the drush pm-update command).
• So far I've been unable to retrieve weather information by ZIP code (I've tried several) but it does work by WOEID.

If not every ZIP code will trigger a result then I'd recommend sticking some help text onto that form field to warn people that potentially they may enter stuff and get no result.

As far as the HTML markup I definitely wouldn't recommend trying to rewrite the plugin itself. But moving that chunk of markup (that gets assembled as var html out to something that could be output through Drupal.settings (that could be altered in a theme or module). Possibly something that keeps a single string of markup in Drupal.settings that would be run through string.replace to replace placeholder tokens with values returned by the plugin.

I hacked around and got an example of something like this working in a gist over here.

(This may be headed outside the target zone for a project application ticket and might be worth moving to the sandbox issue queue instead if you want to discuss it more.)

@larsdesigns, the git clone URL mentioned by you:
git clone --branch 7.x-1.x larsdesigns@git.drupal.org:sandbox/larsdesigns/2203553.git is your personal one. Reviewers won't be able to access your code.

Please fix it (e.g.):
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/larsdesigns/2203553.git simple_weather

Being a minor update to be done, leaving you at "Needs Review" for now!

Looks good, just too bad it only supports US zipcodes :)

Just a minor issue; instead of calling 'simple_weather_file_required' on each block view, why not implement a hook_requirements to check for the library?

After installing the module I got the message saying I should install the simpleWeather library.

After installing the library the message only disappeared once I'd cleared the cache. Maybe the message should say install library and then clear the cache?

I entered my WOEID of 29062 for Milton Keynes in the UK. The H2 subtitle on my weather block says "Milton Keynes, " (trailing comma and whitespace).

The comma on the end without any other details is not so pretty. Maybe check if there is not a second part of the address and remove the comma and trailing space if it is not present?

One other idea was whilst it is very easy to change the background color of the image with css maybe put it as a configuration option of the block so people can change the appearance without having to edit any css?

As these are only minor points I'm not going to change the status of the project.

The "Simple Weather Block Title" field appears on every block configure form - not just yours.

This code is at fault:

if (isset($form['delta']['#value']) == 'simple_weather_report') {
    $form['settings']['title']['#title'] = t('Simple Weather Block Title');
}

Remove the call to isset() and you'll be ok.

Sorry, I didn't explain myself very well.

If $form['delta']['#value'] is set (which is on every block config form) then isset will return TRUE.

TRUE == 'simple_weather_report' evaluates to true, so your code is changing the title on all block config forms (try to edit a different block - you will see title field is labeled Simple Weather Block Title).

You need to check if isset first, and then go on to also check that the strings match:

if (isset($form['delta']['#value']) && $form['delta']['#value'] == 'simple_weather_report') {
    $form['settings']['title']['#title'] = t('Simple Weather Block Title');
}

Reviewed and tested and looks all good to me :)

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/simple_weather.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   5 | ERROR | Doc comment short description must be on a single line, further
     |       | text should be a separate paragraph
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:

1. project page: should mention differences to existing projects such as https://drupal.org/project/weather or https://drupal.org/project/weather_block or https://drupal.org/project/wunderground_weather
2. simple_weather.js: "Yahoo! Weather Forcast" is user facing text and must run through Drupal.t() for translation.
3. simple_weather_content(): instead of using drupal_add_js/css() you should build and return a render array here and use #attached to add you JS/CSS. Then you also don't need to invoke theme() directly and other modules can alter the render array in a meaningful matter. See the topic on render arrays: https://drupal.org/node/930760
4. simple_weather_form_alter(): why don't you check the form ID here? This targets all form ever generated by your Drupal install. You want hook_form_FORM_ID_alter() instead.

But that are not critical application blockers, so ...

Thanks for your contribution, larsdesigns!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.