Update manager #23
No reviewers
Labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Miroslavsckaya/tg_rss_bot#23
Loading…
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 0
return 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 = url
self.title = title
self.description = description
self.date = date
Do you really need a date here?
@ -11,3 +11,4 @@
class Feed:
def __init__(self, url: str, items: list[FeedItem]) -> None:
self.url = url
self.items = items
By the way, you can move all logic from
RssReader.__get_items()
andRssReader.__convert_to_feed_item()
to theFeedItem
itself.If
Feed
knows what to do with URL,FeedItem
could'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 = database
Typehints 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).items
We'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 @@
continue
chat_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_items
diff = []
urls = list(map(lambda x: x[1], old_items))
The same as above goes here.
@ -0,0 +38,4 @@
diff.append(item)
return diff
So 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 = title
self.description = description
class 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).items
old_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 = ''
None
could 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
href
attribute value here.http://htmlbook.ru/html/a/href
@ -91,3 +91,3 @@
"""Sends notifications to users about new RSS feed items."""
BATCH_LIMIT: int = 30
BATCH_LIMIT: int = 29
Why?
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