Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Be a little friendlier to the Reddit API #125

Closed
wants to merge 12 commits into from
Closed

Be a little friendlier to the Reddit API #125

wants to merge 12 commits into from

Conversation

Scriptim
Copy link
Contributor

Description

  • A request to the Reddit API sends a user agent string that conforms to the Reddit API rules.
  • Posts from multiple subreddits to be sent to the same Discord text channel are all retrieved with a single request, thereby reducing the total number of requests.

Issue
This potentially resolves #124.

Additional Notes
The number of requests can be further optimized in such a way that all posts are retrieved at once. In this case, the mapping to the correct Discord text channels would have to be done subsequently.

Regarding the settings for the cog, the following changes:

  • A Reddit user must be set to be named as the author of the application (i. e. the bot) in the user agent.
  • The limit changes its meaning from a per-subreddit limit to a per-textchannel limit.

integrations/reddit/cog.py Outdated Show resolved Hide resolved
integrations/reddit/cog.py Outdated Show resolved Hide resolved
integrations/reddit/cog.py Outdated Show resolved Hide resolved
@Tert0
Copy link
Member

Tert0 commented Sep 26, 2021

Note:
Don’t create a session per request. Most likely you need a session per application which performs all requests altogether.
More complex cases may require a session per site, e.g. one for Github and other one for Facebook APIs. Anyway making a session for every request is a very bad idea.
A session contains a connection pool inside. Connection reusage and keep-alives (both are on by default) may speed up total performance.

https://docs.aiohttp.org/en/stable/client_quickstart.html

@Scriptim Scriptim marked this pull request as ready for review September 27, 2021 21:50
@Scriptim Scriptim requested review from a team as code owners September 27, 2021 21:50
integrations/reddit/cog.py Outdated Show resolved Hide resolved
integrations/reddit/cog.py Show resolved Hide resolved
integrations/reddit/cog.py Show resolved Hide resolved
@Defelo Defelo self-requested a review October 1, 2021 10:12
@codeclimate
Copy link

codeclimate bot commented Oct 4, 2021

Code Climate has analyzed commit f0b2b77 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@Defelo Defelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit changes its meaning from a per-subreddit limit to a per-textchannel limit.

Would it be possible to change that back? Or split the posts evenly between the subreddits, so that not x posts are taken from the first subreddit and the others are ignored?

integrations/reddit/cog.py Outdated Show resolved Hide resolved
@Defelo Defelo added priority: medium bug Something isn't working labels Jan 3, 2022
@Scriptim
Copy link
Contributor Author

Scriptim commented Apr 11, 2022

The limit changes its meaning from a per-subreddit limit to a per-textchannel limit.

Would it be possible to change that back? Or split the posts evenly between the subreddits, so that not x posts are taken from the first subreddit and the others are ignored?

It's not that straightforward. The new API calls fetch posts from multiple subreddits at once by having the subreddits joined together in the URL with a plus. See for example here. It is beyond our control how the posts are distributed among the different subreddits, though I guess the distribution is quite fair. In theory, you could rank the posts yourself again, so that they are evenly distributed. In this case, however, you would have to either query more posts than you actually want to send, or query posts repeatedly until you have gathered enough. In a way, this would defeat the purpose of this pull request. I don't think there is a reasonable solution to implement a per-subreddit limit without doing unnecessary API requests again. So I' d advocate leaving it at the per-textchannel limit.

@Scriptim
Copy link
Contributor Author

Hey, jfyi: I will not continue to work on this pull request. If you have any change requests, you'll either have to implement them yourself or simply close the pull request (if I eventually get the impression that it's going stale, I'll close it on my own).

@Scriptim Scriptim closed this Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working priority: medium
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Reddit Integration: 429 Too Many Requests Errors
4 participants