Open Weather

Hi Kherda,

the demo looks very slick. Nice!
However, your git command you posted is wrong. It asks for a password.
Please write the correct command:

git clone http://git.drupal.org/sandbox/kherda/1777890.git open_weather

I ran CodeSniffer on your files and there are still some coding issues. They are below.
Please find the report as attachment in my next comment below. I couldn't attach something here while re-editing.
You can check with CodeSniffer yourself: http://drupal.org/project/drupalcs

It is better if these code styling issues are fixed first.
The guidelines for code writing can be found here: http://drupal.org/coding-standards

cheers,

Joost

@jvdurme

Please don't paste errors as comment. Attach them in a document if they are more than 20.

thanks

Ah, ok. Sorry about that.

Kherda, find the CodeSniffer report attached to this comment.

Hi,

this is a very nice module, thanks!

Some challange was to start poperly working.
I do not know that just for me or not, but can't generate weather data on times what in admin form have:
2 am 5 am 8 am 11 am 2 pm 5 pm 8 pm 11 pm

I get these times :
3 am 6 am 9 am 12 pm 3 pm 6 pm 9 pm 12 am

After modify times with timezone differeces works fine.

Here a patch, maybe help somebody too.

Regards,
Joe

Hi Kevin,

Thanks for your really interesting project, I hope that will be approve early, so please update your project follow Drupal Coding Standard. And I also please to give some small note after take a look your code:

  $output->list[0]->id;

- should we add a litle validate here to skip some special cases

  $db = db_query("SELECT timestamp FROM {open_weather} LIMIT 1")->fetchObject();

- Could we we use fetchField here

function open_weather_cron() {

  $db = db_query("SELECT timestamp FROM {open_weather} LIMIT 1")->fetchObject();
  if (is_object($db)) {
    $expiretime = $db->timestamp + (variable_get('open_weather_refresh') * 60);
    if (time() > $expiretime) {
      open_weather_fill_weather();
    }
  }

  return;
}

/**
 * Clears old weather, stores new, logs it to watchdog.
 */
function open_weather_flush_caches() {

  open_weather_fill_weather();
  watchdog('open_weather', 'Cleared all saved weather and reloaded new weather', $variables = array(), $severity = WATCHDOG_NOTICE, $link = NULL);

  return;
}

- Maybe "return" is useless in these situations

Hope that your project will be approved early.
Regards,
WeebPal

Зlease specify the requirements for the module like CURL library for PHP. I had to install it to see the module ;)

And some bugs:
1. You need to remove a master, and set the correct default branch: Setting a default branch, Moving from a master to a major version branch.

2. Please fix coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxkherda1777890git

3. open_weather.module
Line 89: use t() for the watchdog message.
open_weather.admin.inc
Line 23: use t() for "City", "Coords"...
Line 35: use t() for 'Fahrenheit'

4. Try to not use queries like db_query("DELETE FROM {variable} WHERE name LIKE 'open_weather_%'");. Use

db_delete('variable')
  ->condition('name', db_like('open_weather_') . '%', 'LIKE')
  ->execute();

5. In open_weather_uninstall() function use variable_del() for each variable in case if someone accidentally call its variable starting with 'open_weather_'.

Set this to "needs review" once you are ready.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hello,

I receive this errors

Notice: Undefined variable: blocks in open_weather_block_info() (line 536 of D:\wamp\www\testdr\sites\all\modules\openweather\open_weather.module).
Warning: Invalid argument supplied for foreach() in _block_rehash() (line 389 of D:\wamp\www\testdr\modules\block\block.module).

Thanks,
Marius

Here is a patch for localization.

Yes, it missed for me too. Wind data are international knot and I need them meters per second. If could set it in admin form is better than modify open-weather-block.tpl.php.

Some secure and more localization.
Because t() say:
"You should never use t() to translate variables, such as calling
t($text);
, unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array)."
I do not know this is the best way, but so could translate weather text and description. If it's other way I hope drupal experts will tell us.

Sorry that code and not patch but I hacked much more the module :)

  $city_data = array(
    "city id" => check_plain(@$output->list[0]->id),
    "coordinates" => array(
      "lat" => check_plain(@$output->list[0]->coord->lat),
      "long" => check_plain(@$output->list[0]->coord->lon),
    ),
    "station" => check_plain(@$output->list[0]->name),
    "current weather" => array(
      "temp" => check_plain(@$output->list[0]->main->temp),
      "pressure" => check_plain(@$output->list[0]->main->pressure),
      "humidity" => check_plain(@$output->list[0]->main->humidity),
      "temp_min" => check_plain(@$output->list[0]->main->temp_min),
      "temp_max" => check_plain(@$output->list[0]->main->temp_max),
      "wind" => check_plain(@$output->list[0]->wind->speed),
      "clouds" => check_plain(@$output->list[0]->clouds->all),
      "weather id" => check_plain(@$output->list[0]->weather[0]->id),
      "weather text" => t(@$output->list[0]->weather[0]->main, array(), array('context' => 'open_weather')),
      "weather desc" => t(@$output->list[0]->weather[0]->description, array(), array('context' => 'open_weather')),
      "weather icon" => check_plain(@$output->list[0]->weather[0]->icon),
    ),
    "block" => array(
      'weather icon' => 1,
      'current temp' => 1,
      'city name' => 1,
      'weather conditions' => 1,
      'weather description' => 1,
      'wind' => 1,
      'clouds' => 1,
    ),
  );

Wow, what a great module! Views integration is hugely useful. Overall, I think this is quite worthy to be accepted into the community. There are a few issues I've noted below:

Automated Review:
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
There is a git tag that has the same name as the branch 7.x-1.x. Make sure to remove this tag to avoid confusion.

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

Manual Review:

1. Consider having impertial/metric option on individual block instances AND in the field options in views.
2. README.txt could use some formatting love
3. Can you explain the necessity of hook_init() and the EXCLUDED_PAGES define?
4. open_weather.module line 8 - SECUREPAGES_PAGES?
5. open_weather.module lines 46 and 55 - both menu items point to the same form, maybe there should be only one?
6. open_weather.module line 71, 97 - 'ow' should probably not be abbreviated - "open_weather" is more consistent
7. open_weather.module line 90 - unsure of function naming
8. Cloud/sun imagery - what is the origin? Did you design these or are they available somewhere and can we see the license?
9. open_weather.module lines 606-641 and 787-822 - there has to be a less verbose way of doing this. Maybe a regex?
10. open_weather.views_default.inc has an empty hook_views_default_views implementation
11. open_weather.views_default.inc lines 20-55 - verbose, see above.
12. open_weather.css has some empty selectors

Bottom Line
Aside from a few issues, this is a very well-done module and should make it to full project status.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Sorry to dig up old thread, but here is a working module for OpenLayers and OpenWeatherMap: http://drupal.org/project/olowm

Updated git url and specific module information that has changed or been updated as I have been developing.