edwardslabs / CloudBot

CloudBot - The simple, fast, expandable, open-source Python IRC Bot!
GNU General Public License v3.0
28 stars 31 forks source link

Create deals.py -- a plugin for listing shopping deals #206

Closed typoguy closed 6 years ago

linuxdaemon commented 6 years ago

What about using the main function from feeds.py? instead of duplicating the code

typoguy commented 6 years ago

I use tabs.

feeds.py is a plugin and not part of a core function of the bot, and I think it's silly to have plugins depend on other plugins. If some feedparser-like methods were introduced under the util directory, then that might change my mind as we could suggest any plugins using similar code draw from there first.

feeds.py also has very little in common with my plugin if you compare both of them plainly. There's a few statements that obviously have to be the same (imports, feedparser.parse), but feeds.py is also targeted very specifically and nothing from that plugin could be used in mine without first rewriting it.

Lastly, I don't see the need to create specific feedparser helper functions at all. You're creating extra overhead when the already simple feedparser module functions just fine on its own.

linuxdaemon commented 6 years ago

Ok, well migrating the formatting logic in the command functions to a single function would probably be ideal. Something like:

def get_deals(title, url, limit=3):
    feed = feedparser.parse(url)
    items = (
        "{} ({})".format(item.title, web.try_shorten(item.link))
        for item in feed.entries[:limit]
    )
    deals = ' \u2022 '.join(items)
    return "{}: {}".format(title, deals)

and then you could use

get_deals("meh.com", "https://meh.com/deals.rss", 1)

and

get_deals("slickdeals.net", "https://slickdeals.net/newsearch.php?mode=frontpage&searcharea=deals&searchin=first&rss=1")
typoguy commented 6 years ago

That definitely sounds like a good idea for future proofing the plugin so it makes adding new sites that happen to support RSS easier and require less code long-term.

I'll certainly consider doing that in any future PR's should @edwardslabs merge the plugin.