Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: use reader.get_entry_counts() and limit= for better efficiency #16

Closed
lemon24 opened this issue Jul 9, 2022 · 5 comments · Fixed by #17
Closed

Suggestion: use reader.get_entry_counts() and limit= for better efficiency #16

lemon24 opened this issue Jul 9, 2022 · 5 comments · Fixed by #17

Comments

@lemon24
Copy link

lemon24 commented Jul 9, 2022

(Found this while looking at your code for lemon24/reader#206 (comment) and thought it might be helpful, feel free to ignore me. :)

In check_feeds(), you can use get_entry_counts() and limit= to avoid reading all the entries in memory.

It should be possible to rewrite this:

entries = list(reader.get_entries(feed=feed, sort="recent"))
unread_entires = list(reader.get_entries(feed=feed, sort="recent", read=False))
entries_count = len(entries)
unread_entires_count = len(unread_entires)
if unread_entires:
    if unread_entires_count == entries_count:
        await send_feed_entries(context, feed.title, unread_entires[:5])
    else:
        await send_feed_entries(context, feed.title, unread_entires)
    for entry in unread_entires:
        reader.mark_entry_as_read(entry)

Like this:

counts = reader.get_entry_counts(feed=feed)
unread_count = counts.total - counts.read  # I should add a counts.unread property, my bad
if unread_count:
    if unread_count == counts.total:
        unread_entries = list(reader.get_entries(feed=feed, read=False, limit=5))
    else:
        unread_entries = list(reader.get_entries(feed=feed, read=False))
    await send_feed_entries(context, feed.title, unread_entries)
    for entry in unread_entires:
        reader.mark_entry_as_read(entry)

Also, feed.last_updated is None is likely a more reliable way of checking if a feed is new.

@dbrennand
Copy link
Owner

Hey @lemon24

It would be nice if there was a method to mark all entries as read for a feed in one go. Is this possible?

Something like: reader.mark_all_entries_as_read(feed=feed)

dbrennand added a commit that referenced this issue Jul 9, 2022
@dbrennand
Copy link
Owner

dbrennand commented Jul 9, 2022

Hmmm, unless I'm missing something here, I need to read all the feed entries into memory anyways if the feed is newly added?

@dbrennand
Copy link
Owner

dbrennand commented Jul 9, 2022

Hey @lemon24

It would be nice if there was a method to mark all entries as read for a feed in one go. Is this possible?

Something like: reader.mark_all_entries_as_read(feed=feed)

@lemon24, I think I definitely have a strong use-case for this. If there was a method such as this, I wouldn't have to read all the entries into memory. I could just limit to 5 as you suggest and use this method to mark them all read.

@lemon24
Copy link
Author

lemon24 commented Jul 9, 2022

I'm a bit reluctant to add exactly that, since both in the case above, and in a similar case in the reader web app, it'd cause a race condition – if the feed is updated after the get_entries() call, new entries would be mistakenly marked as read.

If I ever add batch methods for performance, I'll keep this in mind, though.

@dbrennand
Copy link
Owner

I'm a bit reluctant to add exactly that, since both in the case above, and in a similar case in the reader web app, it'd cause a race condition – if the feed is updated after the get_entries() call, new entries would be mistakenly marked as read.

If I ever add batch methods for performance, I'll keep this in mind, though.

Ah I see, I didn't think of that! Guess it's fine for now though 🙂 thanks!

@dbrennand dbrennand mentioned this issue Jul 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants