-
Notifications
You must be signed in to change notification settings - Fork 13
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
Track Reddit API rate-limits #18
base: master
Are you sure you want to change the base?
Conversation
Hi and thanks for the PR! Yeah, this project was built way before all of the AI craze and companies locking down their content more aggressively behind API limits. I'll leave very minor suggestions, but the ability to do this is definitely needed. |
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.
Small requests around to improve the maintenance—could you also add an example of running this code? It can go to /examples
with possibly a small snippet from running live, showing the expected behavior.
The main reason for my caution is that I haven't worked with Reddit for many years and all of this web code can't really be unit-tested, so it's nice having a proof that "some point in time this worked."
Thanks!
Stream Streaming | ||
Values RedditVals | ||
Client *http.Client | ||
RateLimitUsed int // The number of requests used in the current rate limit window |
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.
For the godoc and LSPs to work nicely, can you put the inlined comments above the declaration in godoc format of // RateLimitUsed indicates the number of...
@@ -38,6 +39,24 @@ func (c *Reddit) MiraRequest(method string, target string, payload map[string]st | |||
return nil, err | |||
} | |||
defer response.Body.Close() | |||
|
|||
// Reddit returns integers for X-Ratelimit-Used and X-Ratelimit-Reset, but not for X-Ratelimit-Remaining |
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.
Can you link the same link from the PR? So whenever someone looks at the code in the future, they can see where these values are documented and look back on them—say when/if they change in the future.
// Reddit returns integers for X-Ratelimit-Used and X-Ratelimit-Reset, but not for X-Ratelimit-Remaining | ||
if rateLimitUsed := response.Header.Get("X-Ratelimit-Used"); rateLimitUsed != "" { | ||
if used, err := strconv.Atoi(rateLimitUsed); err == nil { | ||
c.RateLimitUsed = used |
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.
Would it make sense to give these some default values other than 0? i.e., is 0 a valid value in regular runtime and could we have something else to signify its "uninitialized" and/or "unknown" (say, whenever err != nil
)? Since all of these are signed, -1?
This should help those who wish to avoid sending unnecessary requests when close to or hitting the Reddit API rate limit. I used the headers Reddit documents here: Reddit API Wiki. The code handles missing or changed headers gracefully, so there isn't a risk of breaking if Reddit changes header names in the future.