The module still does the same as the original module; it implements the API of Yr.no to collect weather forecast data and display it on a Drupal site.
As for the code, the module is completely rewritten. Among other things, this has significantly shortened the amount of code.
To address the major issues from my last application:
- Dependencies are checked on install.
- Output is properly filtered by Drupal before sent to the user (and not on database storage..).
- hook_theme() is implemented, along with template files.
- I have adhered to Drupal coding standards as best I can, and have run my code through the coder.module to check if anything major is awry.
- Licensing issues regarding included image files are no longer a problem, as none are included. Yr.no allows hotlinking to their image resources, as long as a link back to them is displayed, which I do in this module.
Although Yr.no's API is primarily targeted at Norwegian-speaking users, the do deliver their data in English as well, making the target audience for this module the same as any other weather-API.
A demo is available at http://krkh.info/yr_verdata/1 (site is in Norwegian, but the functionality of the module is the same).
Previous applications below:
--------------------------------------------
I recently applied for a CVS account for a module I have written (original cvs application at the bottom of this one). The application was turned down with the message:
"One size can fit all if you are prepared to do the work of integration. But you'd first need to discuss that with the current module maintainer. If they are not interested in expanding their module with external help then another module is possible. But please follow that route first."
While I could contact the maintainer of the "weather" module, I do not believe that integrating my module with that one is a logical way to do it. I did some more looking around, and I found that there are already two other weather forecast modules on drupal.org. In total three, each utilizing a different API (weather uses METAR, yahoo_weather uses Yahoo's weather API and ecweather uses Environment Canada's forecast data). My module implements yet another API, and I see no reason to deny another API to be added in its own module, as is already the case with these others.
Original application below:
--------------------------------------------
I am maintaining a couple of Drupal sites, and in doing so, I have written a small module for one of them. I realized it might be worth sharing, so here is my application.
The module in question is named Verdata, and is a weather forecast module. I realize there is already a similar module here on drupal.org, but this one differs in that it uses another weather service. The service is http://yr.no, which is a Norwegian service, also available in English. There are more than 7 million worldwide locations available from yr.no, even including research stations in Antarctica! A demo of my module is available at http://drupaldev.freka.net/node/4, including the code.
The reason I wanted to create this module was simply because I believe there is room for another weather module. One size does not fit all, and METAR's API differs from that of yr.no. Some parts of the yr.no API are more suited for use in Norway (e.g. licensing on radar images restricts verdata.module from using radar images outside mainland Norway), but I believe there is adequate features in it for people outside Norway to find it useful. Leveraging yr.no's API, it provides short- and long-term forecasts up to one week ahead, for all locations. METAR's API is also currently limited to major airports as far as I know, while yr.no delivers forecast for – as previously mentioned – more than 7 million locations.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | yr_verdata.zip | 18.63 KB | tjodolv |
| #5 | remove_int_and_correct_theme_logic.patch | 1.06 KB | tjodolv |
| #4 | remove_int_cast.patch | 431 bytes | tjodolv |
| #1 | yr_verdata.zip | 24.35 KB | tjodolv |
Comments
Comment #1
tjodolv commentedComment #2
avpadernoPlease change only the status, when you upload a new version of the code; other meta data are not thought to be changed by the applicant (and they are not usually changed).
Comment #3
avpadernoGiving a quick peek to the code I noticed that:
The row title is a concatenation of string, and it is not translatable; as it is used in a link, it would be better to make it translatable. To notice that using
t($row['title'])would not resolve the problem as the first argument of the function needs to be a literal string (not even a concatenation of literal string), otherwise the script used to extract the strings to translate would not extract that string.That code doesn't do anything, as the returned value is not saved in a variable; maybe the code should be
$yid = (int) $yid, even if it's not clear why the variable content should be casted to an integer when PHP does that automatically (if you write$a = 1 + "5isaninteger", PHP will assign 6 to$a).Comment #4
tjodolv commentedThank you for the quick review. As for the issues you mention:
$row['title']variable is built from the actual name of the location, as input by the user. As such, setting up a translation method for this is not doable with Drupal'st(), as there is no literal string to pass as an argument.Comment #5
tjodolv commentedWhoops, I also found a weakness in the logic for resolving if a certain day in the detailed forecast is to be closed or not.
yr_verdata.module, line 246:
Should be:
New patch attached. Disregard previous patch.
Comment #6
avpadernoPlease upload the full archive, not a patch; who reviews the code (there are more than one reviewer) should otherwise get the previous last archive, apply the patch, and review the code.
Comment #7
tjodolv commentedAh, I apologize. Here is a new, complete build.
Comment #8
avpadernoUsing placeholders (using something like
"@type, @altitude m.a.s. <br />Last updated"could be better; the string you are appending could be pre-prended, in some languages.Drupal coding standards suggest to use Drupal Unicode functions when available (see
drupal_strtoupper(),drupal_strtolower(), etc...).Put a space before and after
&&.The URLs should be put in the string through placeholders, if there isn't a different link for each supported languages; to make an example, if I would report a link to the function
ucfirstin PHP.net, I would put it directly into the string (as the English version would use a link to http://www.php.net/manual/en/function.ucfirst.php, while the German version would use http://www.php.net/manual/de/function.ucfirst.php)Normally, the used string would be
'Warning: You are about to delete the location %location from the database. This action cannot be undone.'It should be
Comment #9
tjodolv commentedThank you for your time! I have corrected the issues you mentioned in your last post.
Again, thank you for your time and effort.
Comment #12
avpaderno