Update manager #23
No reviewers
Labels
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Miroslavsckaya/tg_rss_bot!23
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature_rss_update"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
WIP:feature_rss_updateto WIP:Update managerLooks fine, but some things could be improved.
@ -3,6 +3,7 @@# Python/.venv/__pycache__Why? What is it for?
@ -97,6 +98,11 @@ class Database:return 0return int(row[0])def find_feed_subscribers(self, feed_id: int) -> list:Maybe we can use more specific type-hint here.
@ -6,3 +6,4 @@self.url = urlself.title = titleself.description = descriptionself.date = dateDo you really need a date here?
@ -11,3 +11,4 @@class Feed:def __init__(self, url: str, items: list[FeedItem]) -> None:self.url = urlself.items = itemsBy the way, you can move all logic from
RssReader.__get_items()andRssReader.__convert_to_feed_item()to theFeedItemitself.If
Feedknows what to do with URL,FeedItemcould've known how to initialize itself from the dictionary.// It's not a problem, just a suggestion of an alternative.
// You can resolve this thread yourself if you don't want to implement that this way.
@ -0,0 +5,4 @@class UpdateManager:def __init__(self, database: Database, notifier: Notifier, rss_reader: RssReader) -> None:self.database = databaseTypehints for properties would also be good too.
@ -0,0 +13,4 @@feeds = self.database.find_feeds()for feed_id, feed_url in feeds:new_items = self.rss_reader.get_feed(feed_url).itemsWe're forgetting about the feed itself creating small UX problem for the bot user here. But it can be addressed later.
@ -0,0 +22,4 @@continuechat_ids = self.database.find_feed_subscribers(feed_id)chat_ids = list(map(lambda x: x[0], chat_ids))Why would you do that instead of just returning ready-for-use list of ID's from the
Database?@ -0,0 +31,4 @@return new_itemsdiff = []urls = list(map(lambda x: x[1], old_items))The same as above goes here.
@ -0,0 +38,4 @@diff.append(item)return diffSo many space in the end of the file. Is it reserved for future improvements? 😄
There still an entrypoint script to implement besides other fixes by the way.
Better, but still some things to improve.
@ -7,2 +4,2 @@self.title = titleself.description = descriptionclass FeedItem:def __init__(self, item: feedparser.util.FeedParserDict) -> None:Just import
FeedParserDict.@ -29,2 +14,3 @@self.items = self.__get_items(feed.entries)def __get_items(self, items: list) -> list:def __get_items(self, items: list[feedparser.util.FeedParserDict]) -> list[FeedItem]:It's just 3 lines. You can just do that in the constructor.
@ -0,0 +14,4 @@for feed_id, feed_url in feeds:new_items = self.rss_reader.get_feed(feed_url).itemsold_items_url = self.database.find_feed_items_url(feed_id)old_items_urls(plural, same goes for method name)Nice. Now it should work, but we can still improve some things.
@ -9,0 +9,4 @@if 'published' in item:self.date = item.published_parsed()else:self.date = ''Nonecould probably be better here if we hadn't parse the date.@ -60,3 +60,2 @@feed_list = ''for feed in feeds:feed_list += '* ' + str(feed[0]) + ': ' + feed[1] + '\n'for count, feed in enumerate(feeds, start=1):It's better to use "number" or "index" here :)
@ -61,2 +61,2 @@for feed in feeds:feed_list += '* ' + str(feed[0]) + ': ' + feed[1] + '\n'for count, feed in enumerate(feeds, start=1):feed_list += '* ' + str(count) + ': ' + feed['url'] + '\n'If we're using title now, we can show the title here and make it a link to the feed URL.
@ -101,2 +100,4 @@def send_updates(self, chat_ids: list[int], updates: list[FeedItem], feed_title: str):"""Send notification about new items to the user"""for chat_id in chat_ids:self.bot.send_message(Do not forget about incrementing sent messages counter after sending the message.
@ -121,2 +125,3 @@f"<strong><a href=\"{item.url}\">{item.title}</a></strong>\n\n"f"{item.description}"f"{item.description}\n"f"{item.date}"I'd suggest to place a date after the feed item title:
Or HTML in our case... We've switched to it some time ago.
@ -144,7 +157,6 @@ class Database:' url TEXT NOT NULL UNIQUE,'' title TEXT,'' description TEXT,'' date NUMERIC,'Now as we don't have migrations, to apply this change we either need to remove the database or execute the change statement manually.
It's just a note, not a change request.
I'd say that we can release 0.1 as soon as you fix small HTML syntax error in
telegram.py🙂@ -61,2 +61,2 @@for feed in feeds:feed_list += '* ' + str(feed[0]) + ': ' + feed[1] + '\n'for index, feed in enumerate(feeds, start=1):feed_list += '* ' + str(index) + ': ' + f"<a href={feed['url']}>{feed['title']}</a" + '\n'You forgot the quotes surrounding
hrefattribute value here.http://htmlbook.ru/html/a/href
@ -91,3 +91,3 @@"""Sends notifications to users about new RSS feed items."""BATCH_LIMIT: int = 30BATCH_LIMIT: int = 29Why?
If you have time you can also make some changes PyLama suggests. But we can also do that later.
LGTM 👍
WIP:Update managerto Update manager