Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This module retrieves weather data from the Wunderground API and display this in two block, one for current weather conditions and one for weather forecasts. The data is obtained via a REST call and is cached, so that the number of call to the API is limited. It contains and easy settings form to fill in the api key and to select the location for which the weather data should be displayed.
Project page: https://drupal.org/sandbox/jeroen_drenth/2104663
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jeroen_drenth/2104663.git wunderground_weather
Comment | File | Size | Author |
---|---|---|---|
#15 | Importunus Nutus Proprius contrib.local_.png | 10.51 KB | AjitS |
#15 | Importunus Nutus Proprius contrib.local_.png | 23.66 KB | AjitS |
Comments
Comment #1
AjitSHi, please change the git link in your original post for non-maintainers. Cloning using the original link asks for your d.o user password.
Also, check for the issues reported by pareview :
http://pareview.sh/pareview/httpgitdrupalorgsandboxjeroendrenth2104663git
Comment #1.0
jeroen_drenth CreditAttribution: jeroen_drenth commentedchanged git link for non-maintainers.
Comment #1.1
jeroen_drenth CreditAttribution: jeroen_drenth commentedchanged git to branche.
Comment #2
jeroen_drenth CreditAttribution: jeroen_drenth commentedHi AjitS,
I've changed the link and also solved the issues reported by pareview.
Comment #3
jeroen_drenth CreditAttribution: jeroen_drenth commentedComment #4
t14 CreditAttribution: t14 commentedHi
You have a few spelling errors in your comments on line 81, 113, 130.
You may want to use filter_xss() at some some point on the data you are obtaining from the API calls. Just to be on the safe side.
Personally I would cache the data rather than store it in your own custom tables. This would be a better light weight solution than creating a hook_schema on install and relying on a cron job. It will also allow you to set a time when you want the cache to expire and make a new API call. You can set it within the code or make it possible for your users to decide via you admin settings form.
So rather than save to the database on wunderground_weather_get_forecast_data() and wunderground_weather_get_current_data() you can make use of cache_set and cache_get ()
https://api.drupal.org/api/drupal/includes!cache.inc/function/cache_set/7
https://api.drupal.org/api/drupal/includes!cache.inc/function/cache_get/7
Much less code and more functionality.
If you do decide to stick with the cron method, you will need to remove the “TODO” in your schema definitions and give a valid description for each field. You may also want to consider implementing hook_cron_queue_info to queue your cron jobs. Will allow your functionality to work successfully in something like a big site that has a lot of task to do during a cron job, hence meaning that your cron job may not run if cron timeouts.
Here is a good tutorial for implementing hook_cron_queue_info
http://www.metaltoad.com/blog/handling-long-running-background-tasks-dru...
Hope that was of help to you.
T
Comment #5
jeroen_drenth CreditAttribution: jeroen_drenth commentedHi T,
Thank you for your feedback. It was very useful. The module is now using cache instead of custom tables. The user can also now choose wether he wants to cache the data or not, by checking a box on the settings page.
Comment #5.0
jeroen_drenth CreditAttribution: jeroen_drenth commentedUpdated git link
Comment #6
PA robot CreditAttribution: PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #6.0
PA robot CreditAttribution: PA robot commentedUpdated module description.
Comment #7
Enxebre CreditAttribution: Enxebre commentedHi jeroen_drenth,
I think you are using hook_theme in a wrong way.
There should be just one implementation of hook_theme nor two, this is the correct one for you "wunderground_weather_theme()", line 55.
The "function wunderground_weather_forecast_theme " should be called "function theme_wunderground_weather_forecast($variables)"
Then You will be able to generate the html calling
$your_html = theme('wunderground_weather_forecast, $variables)
or creating a render element (with #properties) which matchs with your definition in hook_theme and passing it through "drupal_render " function
https://drupal.org/node/933976
Hope this helps.
Regards!
Comment #8
Enxebre CreditAttribution: Enxebre commentedComment #9
jeroen_drenth CreditAttribution: jeroen_drenth commentedThank you, Enxebre. I've changed the way the module uses hook_theme.
Comment #10
AjitSautomation issues
Code Sniffer found the following issues.
manual review
You have a form function which will set the variables on form submit. You should also have an
.install
file which should implementhook_uninstall()
, deleting all the variables usingvariable_del()
.In the
.info
file, use only single full stop. Not a blocker but it is good to keep things clean :-)In
.module
'shook_menu
implementation, there is no need of/
for the'file'
key.Rest all looks good.
Comment #11
jeroen_drenth CreditAttribution: jeroen_drenth commentedThanks Ajits. I added the hook_uninstall and changed the .info file.
Comment #12
AjitSThese errors occured randomly, I got it at
admin/config/user-interface/wunderground-weather
menu item.You should change the
to
which provides default value to
$text
and get rid of the warnings. These are not blockers, hence not changing the status to needs work.Comment #13
jeroen_drenth CreditAttribution: jeroen_drenth commentedThank you again! Changed it to $text = ''.
Comment #14
AjitSTested this functionally, and works good. The only thing I notice is incomplete is README.txt file.
Please read Module documentation guidelines, on how the README.txt should be.
Also, you show help in UI by implementing
hook_help()
in your.module
file. Also, some screenshots on the project page would be great.Other than that this looks like RTBC to me :-) Good work.
Comment #15
AjitSOn further testing, I want to add some points to the above review:
When I don't input any Display name in the settings form. I see something like this in the screenshot in attachment #2 below (was not able to bring that up in the editor).
It shows a message like:
above the block. This is ambiguous. To avoid this, you should mark this field as
'#required' => TRUE,
in yourhook_form()
implementation.Some suggestions for the fields that require some changes IMO are
In screenshot #1, there is a blank in the first list element. Should either be populated with something, or the element should be removed when no value is present.
I think this will be a very useful module. Might end up using this in some of the projects. Nice work :-)
Comment #16
jeroen_drenth CreditAttribution: jeroen_drenth commentedOnce again some very useful comments. Thanks again. I have altered the settings form as you suggested.
Comment #17
AjitSLooks good. RTBC from my side :-)
Just one thing though, I think it would make much more sense if you move the configuration under Web Services (
admin/config/services
); as it makes use of an external service.Lets wait for some other reviews too. You might want to apply for a review bonus to speed things up.
Comment #18
robinvdvleuten CreditAttribution: robinvdvleuten commentedLooks good to me!
Comment #19
kscheirertheme('table', $variables);
. That lets theme overrides work properly. If you call the function directly (theme_table()) then no overrides are possible. Same with theme_item_list() and theme_image().Thanks for your contribution, jeroen_drenth!
I updated your account to let you 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:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.
----
Top Shelf Modules - Crafted, Curated, Contributed.
Comment #20.0
(not verified) CreditAttribution: commentedHighlighted git