-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Clarify ApiInfo rate limiting usage in docs #1524
Clarify ApiInfo rate limiting usage in docs #1524
Conversation
I think breaking it up is good, so we can include the basic information asap |
var rateLimit = apiInfo?.RateLimit; | ||
|
||
var howManyRequestsCanIMakePerHour = rateLimit?.Limit; | ||
var howManyRequestsToIHaveLeft = 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.
ToIHaveLeft => DoIHaveLeft
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.
👍 Nice catch, thanks.
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.
Fixed.
@@ -69,3 +69,22 @@ If you've authenticated as a given user, you can query their details directly: | |||
``` | |||
var user = await client.User.Current(); | |||
``` | |||
|
|||
### Too Much of a Good Thing: Dealing with API Rate Limits | |||
Like any popular API, Github needs to throttle some requests. The OctoKit.NET client allows you to get some insight into how many requests you have left and when you can start making requests again. It does this via the `ApiInfo` object and the `GetLastApiInfo()` method. |
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.
Maybe add a hyperlink to the github API docs "rate limiting" section? Also maybe it's worth mentioning that an authenticated client has a much higher limit than anonymous
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.
👍 Great feedback; will do.
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 has been done.
@ryangribble based on your review: ✅ Added link to Github API The build seems to be failing on Travis, but that seems unrelated since I didn't modify the build script. :) |
|
||
An authenticated client will have a higher limit than an anonymous client. | ||
|
||
For more information on the API and understanding rate limits, you may want to consult [the Github API docs on rate limits](https://developer.github.com/v3/rate_limit/). |
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 it would be better to link to this page since it provides info about rate limits etc, rather than the link you had (which is the doc for the actual "Get Rate Limit" API call)
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.
@ryangribble done.
var whenDoesTheLimitReset = rateLimit?.Reset; | ||
``` | ||
|
||
An authenticated client will have a higher limit than an anonymous client. |
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.
Let's say "a significantly higher limit" here since its's a big difference
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.
👍 done.
@ryangribble ready for review again at your leisure. Thanks for taking the time to be so thorough. And happy new year! |
…teLimitInfoToDocs
Awesome stuff, thanks so much for the additional clarity in the docs and congrats @SeanKilleen on the first merged contribution of 2017! Happy New Year 🎉 |
Supports #1142.
Remaining Work
If you'd like to break this PR up at all to get some of the basic info in, let me know and I'll switch it up.