Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
27 Feb 2012 at 19:14 UTC
Updated:
30 Nov 2012 at 19:16 UTC
Jump to comment: Most recent file
Comments
Comment #1
targoo commentedIt appears you are working in the "master" 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.
Review of the master branch:
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. Get a review bonus and we will come back to your application sooner.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #2
patrickd commentedwelcome,
while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools, as pointed out above.
At least moving to a version specific branch should be done before switching back to "needs review".
You can also get a review bonus and we will come back to your application sooner.
regards
Comment #3
migmedia commentedYou add a little more explaining to your project-page:
Are there dependencies between the different modules?
Why is every feed-tag a single module, wouldn't it be cleaner to combine them in a single or core-module and extentions?
Micha
Comment #4
RyanParsley commentedMicha,
It made sense to me to have each tag be a separate because they're optional from Apple's perspective and enabling them fills up the "Manage Display" "Format" select list. Separate modules seemed like the way to let users keep that list uncluttered from elements they don't plan to use. I'm not convinced this is the best approach... but that's what I was thinking at the time.
I'll get on the rest of the changes soon. I only just realized these 3 issues exist. I must need to adjust my email preferences so I get notified.
Since writing this module I discovered http://drupal.org/project/views_rss_itunes. I'm concerned that mine is redundant. The main difference between the two is mine does not depend on views_rss and works with the way core manipulates feeds. A nice thing about choosing Views RSS is it allows for adding all the nice iTunes elements to the feed itself. Using my module, I did this via a .tpl.php. Do you think both need to exist?
Thanks,
Ryan
Comment #5
migmedia commented@Ryan thank you for your efforts.
The way creating feeds by the template-system may be more customizable for the pros, but imho the beginner (as I) would prefere the views_rss_tunes way.
By the way, they have created a nice documentation, what would be a lot of work for a module like yours.
Micha
Comment #6
mitchell commentedHi, RyanParsley. This is a very nice contribution. Thanks for submitting it for review as your first full project!
Here are the issues I found with your code:
* Your project is named itunes_feed_formatters, yet your module namespace is rss_itunes_*
* You're using separate modules for each field formatter. These can all be merged into a single module, and preferably module-name_views.inc
* Your project name doesn't use the views_ namespace.
* An view export based on your code would be tremendously helpful for review. (Also a similar one of the other module for comparison wouldn't hurt.)
Views RSS iTunes hasn't fully taken off yet, so I recommend you either continue working on this code or help improve that project. Do either of those options appeal to you? How would you like to move forward?
Comment #7
klausiClosing due to lack of activity. Feel free to reopen if you are still working on this application.