Update manager #23

Merged
Miroslavsckaya merged 20 commits from feature_rss_update into master 2022-07-08 19:23:12 +00:00
No description provided.
Miroslavsckaya added 2 commits 2022-06-11 20:10:00 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
94d77a42f4
implement update manager
Miroslavsckaya changed title from WIP:feature_rss_update to WIP:Update manager 2022-06-11 20:10:16 +00:00
skobkin requested changes 2022-06-11 22:46:46 +00:00
skobkin left a comment
Collaborator

Looks fine, but some things could be improved.

Looks fine, but some things could be improved.
@ -3,6 +3,7 @@
# Python
/.venv
/__pycache__
Collaborator

Why? What is it for?

Why? What is it for?
skobkin marked this conversation as resolved
database.py Outdated
@ -97,6 +98,11 @@ class Database:
return 0
return int(row[0])
def find_feed_subscribers(self, feed_id: int) -> list:
Collaborator

Maybe we can use more specific type-hint here.

Maybe we can use more specific type-hint here.
skobkin marked this conversation as resolved
rss.py Outdated
@ -6,3 +6,4 @@
self.url = url
self.title = title
self.description = description
self.date = date
Collaborator

Do you really need a date here?

Do you really need a date here?
skobkin marked this conversation as resolved
rss.py Outdated
@ -11,3 +11,4 @@
class Feed:
def __init__(self, url: str, items: list[FeedItem]) -> None:
self.url = url
self.items = items
Collaborator

By the way, you can move all logic from RssReader.__get_items() and RssReader.__convert_to_feed_item() to the FeedItem 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.

By the way, you can move all logic from `RssReader.__get_items()` and `RssReader.__convert_to_feed_item()` to the `FeedItem` 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.
skobkin marked this conversation as resolved
@ -0,0 +5,4 @@
class UpdateManager:
def __init__(self, database: Database, notifier: Notifier, rss_reader: RssReader) -> None:
self.database = database
Collaborator

Typehints for properties would also be good too.

Typehints for properties would also be good too.
skobkin marked this conversation as resolved
@ -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
Collaborator

We're forgetting about the feed itself creating small UX problem for the bot user here. But it can be addressed later.

We're forgetting about the feed itself creating small UX problem for the bot user here. But it can be addressed later.
skobkin marked this conversation as resolved
@ -0,0 +22,4 @@
continue
chat_ids = self.database.find_feed_subscribers(feed_id)
chat_ids = list(map(lambda x: x[0], chat_ids))
Collaborator

Why would you do that instead of just returning ready-for-use list of ID's from the Database?

Why would you do that instead of just returning ready-for-use list of ID's from the `Database`?
skobkin marked this conversation as resolved
@ -0,0 +31,4 @@
return new_items
diff = []
urls = list(map(lambda x: x[1], old_items))
Collaborator

The same as above goes here.

The same as above goes here.
skobkin marked this conversation as resolved
skobkin reviewed 2022-06-11 22:48:22 +00:00
@ -0,0 +38,4 @@
diff.append(item)
return diff
Collaborator

So many space in the end of the file. Is it reserved for future improvements? 😄

So many space in the end of the file. Is it reserved for future improvements? 😄
skobkin marked this conversation as resolved
Collaborator

There still an entrypoint script to implement besides other fixes by the way.

There still an entrypoint script to implement besides other fixes by the way.
skobkin added this to the MVP 0.1 milestone 2022-06-11 22:58:33 +00:00
Miroslavsckaya was assigned by skobkin 2022-06-11 22:58:41 +00:00
skobkin added the
new feature
label 2022-06-11 23:01:01 +00:00
Miroslavsckaya added 2 commits 2022-06-12 10:19:03 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
b2c49a583a
refactor rss.py
skobkin requested changes 2022-06-12 14:00:48 +00:00
skobkin left a comment
Collaborator

Better, but still some things to improve.

Better, but still some things to improve.
rss.py Outdated
@ -7,2 +4,2 @@
self.title = title
self.description = description
class FeedItem:
def __init__(self, item: feedparser.util.FeedParserDict) -> None:
Collaborator

Just import FeedParserDict.

Just import `FeedParserDict`.
skobkin marked this conversation as resolved
rss.py Outdated
@ -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]:
Collaborator

It's just 3 lines. You can just do that in the constructor.

It's just 3 lines. You can just do that in the constructor.
skobkin marked this conversation as resolved
@ -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)
Collaborator

old_items_urls (plural, same goes for method name)

`old_items_urls` (plural, same goes for method name)
skobkin marked this conversation as resolved
Miroslavsckaya added 1 commit 2022-06-12 14:14:02 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
94c1447093
refactor a bit in rss.py
Miroslavsckaya added 6 commits 2022-06-12 19:52:22 +00:00
skobkin requested changes 2022-06-12 20:17:45 +00:00
skobkin left a comment
Collaborator

Nice. Now it should work, but we can still improve some things.

Nice. Now it should work, but we can still improve some things.
rss.py Outdated
@ -9,0 +9,4 @@
if 'published' in item:
self.date = item.published_parsed()
else:
self.date = ''
Collaborator

None could probably be better here if we hadn't parse the date.

`None` could probably be better here if we hadn't parse the date.
skobkin marked this conversation as resolved
telegram.py Outdated
@ -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):
Collaborator

count

Count Douku

It's better to use "number" or "index" here :)

> count ![Count Douku](https://d33wubrfki0l68.cloudfront.net/87c3b00eba91caf42e44d3d875fee707029779de/c8598/assets/img/products/hot-toys-mms-496-star-wars-ii-aotc-count-dooku/13.jpg) It's better to use "number" or "index" here :)
skobkin marked this conversation as resolved
telegram.py Outdated
@ -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'
Collaborator

If we're using title now, we can show the title here and make it a link to the feed URL.

If we're using title now, we can show the title here and make it a link to the feed URL.
skobkin marked this conversation as resolved
telegram.py Outdated
@ -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(
Collaborator

Do not forget about incrementing sent messages counter after sending the message.

Do not forget about incrementing sent messages counter after sending the message.
skobkin marked this conversation as resolved
telegram.py Outdated
@ -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}"
Collaborator

I'd suggest to place a date after the feed item title:

[title](https://skobk.in/some/post) [2022-06-12]

Once upon a time...
I'd suggest to place a date after the feed item title: ```markdown [title](https://skobk.in/some/post) [2022-06-12] Once upon a time... ```
Collaborator

Or HTML in our case... We've switched to it some time ago.

Or HTML in our case... We've switched to it some time ago.
skobkin reviewed 2022-06-12 20:28:14 +00:00
database.py Outdated
@ -144,7 +157,6 @@ class Database:
' url TEXT NOT NULL UNIQUE,'
' title TEXT,'
' description TEXT,'
' date NUMERIC,'
Collaborator

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.

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.
skobkin marked this conversation as resolved
Miroslavsckaya added 1 commit 2022-07-04 15:38:29 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
4f25c2dd3f
minor fixes
skobkin requested changes 2022-07-05 00:02:05 +00:00
skobkin left a comment
Collaborator

I'd say that we can release 0.1 as soon as you fix small HTML syntax error in telegram.py 🙂

I'd say that we can release 0.1 as soon as you fix small HTML syntax error in `telegram.py` 🙂
telegram.py Outdated
@ -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'
Collaborator

You forgot the quotes surrounding href attribute value here.

http://htmlbook.ru/html/a/href

You forgot the quotes surrounding `href` attribute value here. http://htmlbook.ru/html/a/href
skobkin marked this conversation as resolved
telegram.py Outdated
@ -91,3 +91,3 @@
"""Sends notifications to users about new RSS feed items."""
BATCH_LIMIT: int = 30
BATCH_LIMIT: int = 29
Collaborator

Why?

Why?
skobkin marked this conversation as resolved
Collaborator

If you have time you can also make some changes PyLama suggests. But we can also do that later.

If you have time you can also make some changes PyLama suggests. But we can also do that later.
Miroslavsckaya added 1 commit 2022-07-08 17:16:36 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
d25ca99da1
fix html code
Miroslavsckaya added 2 commits 2022-07-08 18:00:05 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
25af6c2176
little changes in pylama.ini
Miroslavsckaya added 1 commit 2022-07-08 18:04:44 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
53fa8caf1e
little code style changes
Miroslavsckaya added 1 commit 2022-07-08 18:09:14 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
5bb83c2a09
change max_line_length in pylama.ini
Miroslavsckaya requested review from skobkin 2022-07-08 18:17:04 +00:00
Miroslavsckaya added 1 commit 2022-07-08 18:49:19 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
8a4bec01c1
refactor send_updates method in Notifier class and add __register_request_and_wait method
Miroslavsckaya added 1 commit 2022-07-08 19:09:15 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is failing Details
c4809a20ea
rename __register_request_and_wait method to __count_request_and_wait and add counter increment to
Miroslavsckaya added 1 commit 2022-07-08 19:15:59 +00:00
continuous-integration/drone/push Build is passing Details
continuous-integration/drone/pr Build is passing Details
1749bd5f5e
fix method calls
skobkin approved these changes 2022-07-08 19:20:21 +00:00
skobkin left a comment
Collaborator

LGTM 👍

LGTM 👍
Miroslavsckaya changed title from WIP:Update manager to Update manager 2022-07-08 19:20:44 +00:00
Miroslavsckaya merged commit 78467bc5fb into master 2022-07-08 19:23:12 +00:00
Miroslavsckaya referenced this issue from a commit 2022-07-08 19:23:13 +00:00
Miroslavsckaya deleted branch feature_rss_update 2022-07-08 19:24:03 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Miroslavsckaya/tg_rss_bot#23
No description provided.