-
-
Notifications
You must be signed in to change notification settings - Fork 458
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
Window size config #1975
Window size config #1975
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes.
handler.setLevel(logging.DEBUG) | ||
stream_handler = logging.StreamHandler() | ||
stream_handler.setLevel(logging.DEBUG) | ||
file_handler = logging.handlers.RotatingFileHandler( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be too advanced. Maybe add a second example explaining why you recommend a rotating file handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
Data: None | ||
Params: {'raw_json': 1} | ||
Response: 200 (876 bytes) | ||
Response: 200 (876 bytes) (rst-45:rem-892:used-104 ratelimit) at 1691743156.3847592 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is from prawcore but what do you think about changing the epoch to a timestamp? It would only be 1 or 2 more characters and would be human readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this to match the timestamp in the first line. But no real opinion whether they should both be timestamps or a human readable date.
@@ -72,6 +72,7 @@ order to successfully access a third-party Reddit site: | |||
(default: ``t3_``). | |||
:subreddit_kind: The type prefix for subreddits on the :class:`.Reddit` instance | |||
(default: ``t5_``). | |||
:window_size: The number of seconds between rate limit resets (default: 600). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be in a different section. This section is for configuring PRAW with a custom Reddit instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
prawcore has been updated. Could you rebase and squash so the pipeline can run again? |
This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
Still planning on finishing this, just haven't had time recently. |
This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
Bump, will have time for this over the holidays if not before. |
This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
Bump |
This PR is stale because it has been open for 30 days with no activity. Remove the Stale label or comment or this will be closed in 30 days. |
Just checking in on this. |
It's still on my list, but pretty far at the bottom below other priorities. How much do you think it's actually needed? Reddit doesn't look like they are making any changes to the window size anytime soon. |
Was waiting on doing a 8.0.0 release for this. Would different paid tiers of rate limits make a difference in the rate limit calculation for optimal handling? |
Extremely unlikely they would have different window sizes. They just increase the requests per window. I imagine a lot of their infrastructure is built with the 10 minute window in mind. |
Addressed comments and got the build passing. I haven't touched this in a year, so I don't really remember my original thought process and whether I was still going to do anything else. Let me know if there's any more changes you want, but no promises how fast I'll get to them. |
Woot and yeah same here it can be changed if needed later on. |
Super bare bones initial implementation of passing a config value for window size into prawcore. Just wanted to have something down so I can feel like I started on it.