RSS module refactoring

[Bharat Mediratta]

Hey Tim.  I looked over the rss changes and had some comments:

1) It'd be nice to migrate Rss_Controller away from using __call.
Perhaps use feed() instead.  This would change our urls from
/rss/comments to /rss/feed/comments which I think is fine.

2) Naming:
  rss::get_feeds() -> rss::available_feeds()
  rss::process_feed() -> rss::feed_data()

3) rss::item_feed() and rss::tag_feed() are out of place now because
they have domain knowledge.  You probably want to create
tag_theme::head() which generates the /rss/feed/tags url if the rss
module is active.

4) rss::get_feeds() shouldn't know about the sidebar imo.  I assume
you're doing this so that you can have hidden feeds such that the
slideshow module can have its own, right?  In that case let's just have
the slideshow module inject its own feed url using slideshow_theme.
That avoids leaking display info into the code here and resolves our
dependency issue.  So then you can get rid of all the "sidebar"
references everywhere

5) in the various feed functions, you have $feed as a stdClass, but then
you have a $data array inside it.  Why not just use stdClass?  Eg:

  $feed->title
  $feed->description
  $feed->children = array()  // since this has an ordering

etc.

that's all I've got for now.  This is definitely headed in the right
direction IMO.

-Bharat


------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing
server and web deployment.
http://p.sf.net/sfu/businessobjects
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]

[Tim Almdal]

Overall I have to agree with all of them.
1) Good idea
2) np, you know me and class/method naming :-)
3) Agreed, that was the "wee bit more" refactoring I was referring to the other day.  My view is that, other than the gallery module and there is a user module, modules should not make any assumptions about what other modules are installed.
4) It was only there in terms of the ticket for creating an rss ui to set the default feed.  I wanted to be able to call this helper function and then filter out any that were sidebar only.  But since we postponed that ticket, I can remove it.
5) Good question.  The answer is:  that was left over from when I was using the event mechanism to get list of feeds.

Bharat Mediratta wrote:

Hey Tim.  I looked over the rss changes and had some comments:

1) It'd be nice to migrate Rss_Controller away from using __call.
Perhaps use feed() instead.  This would change our urls from
/rss/comments to /rss/feed/comments which I think is fine.

2) Naming:
  rss::get_feeds() -> rss::available_feeds()
  rss::process_feed() -> rss::feed_data()

3) rss::item_feed() and rss::tag_feed() are out of place now because
they have domain knowledge.  You probably want to create
tag_theme::head() which generates the /rss/feed/tags url if the rss
module is active.

4) rss::get_feeds() shouldn't know about the sidebar imo.  I assume
you're doing this so that you can have hidden feeds such that the
slideshow module can have its own, right?  In that case let's just have
the slideshow module inject its own feed url using slideshow_theme.
That avoids leaking display info into the code here and resolves our
dependency issue.  So then you can get rid of all the "sidebar"
references everywhere

5) in the various feed functions, you have $feed as a stdClass, but then
you have a $data array inside it.  Why not just use stdClass?  Eg:

  $feed->title
  $feed->description
  $feed->children = array()  // since this has an ordering

etc.

that's all I've got for now.  This is definitely headed in the right
direction IMO.

-Bharat
  

No virus found in this incoming message.
Checked by AVG - www.avg.com 
Version: 8.5.339 / Virus Database: 270.12.68/2175 - Release Date: 06/14/09 05:53:00

  

------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing
server and web deployment.
http://p.sf.net/sfu/businessobjects
__[ g a l l e r y - d e v e l ]_________________________

[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]