Skip to content

With removal of deprecated client.Rate in #555, rate limit info needs more visibility #571

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

Closed
gmlewis opened this issue Mar 3, 2017 · 5 comments
Assignees

Comments

@gmlewis
Copy link
Collaborator

gmlewis commented Mar 3, 2017

It used to be that clients might make API calls like:

	repos, resp, err := client.Repositories.ListByOrg(org, opt)
	if err != nil {
		rate := client.Rate()
		if rate.Remaining == 0 {
			//...
		}
		//...
	}

but after #555, it turns out that checkRateLimitBeforeDo will return an error without rate information.
I have a PR on the way that addresses this issue.

@gmlewis gmlewis self-assigned this Mar 3, 2017
gmlewis added a commit to gmlewis/go-github that referenced this issue Mar 3, 2017
Fixes google#571.

Change-Id: Iefcc687d9fc56c40964d5e6f1cdbc54d020dae3c
@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 3, 2017

This change will make it easier and more consistent to check for a rate limit error, using:

if resp != nil && resp.Rate.Remaining == 0 {
	//...
}

@dmitshur
Copy link
Member

dmitshur commented Mar 4, 2017

Just checking, but did you see this sentence in the docs of Do?

If rate limit is exceeded and reset time is in the future, Do returns *RateLimitError immediately without making a network API call.

Also, the section on rate limiting in the package comment, and the example there?

https://godoc.org/github.com/google/go-github/github#hdr-Rate_Limiting

So, the way I'd check for rate limit errors is as shown in the provided example in the package documentation at https://godoc.org/github.com/google/go-github/github#hdr-Rate_Limiting:

repos, resp, err := client.Repositories.ListByOrg(org, opt)
if err != nil {
	if _, ok := err.(*github.RateLimitError); ok {
		// ...
	}
	// ...
}

I wanted to ask these questions first before I can comment further.

@dmitshur
Copy link
Member

dmitshur commented Mar 4, 2017

That section needs to be updated, since it still refers to a removed deprecated method:

The Rate method on a client returns the rate limit information based on the most recent API call.

@gmlewis
Copy link
Collaborator Author

gmlewis commented Mar 4, 2017

Ah, I see. Hmmm... How do you feel about supporting both mechanisms?
I rather like the change in case someone is already used to checking the rate limit in the response... but if you feel strongly, I will just close this CL.

@dmitshur
Copy link
Member

dmitshur commented Mar 4, 2017

@gmlewis I replied in the PR #572 (review), because I wanted to reference specific lines of code there.

gmlewis added a commit that referenced this issue Apr 21, 2017
…Do` (#572)

Return rate limit information immediately after checkRateLimitBeforeDo

Fixes #571.
bubg-dev pushed a commit to bubg-dev/go-github that referenced this issue Jun 16, 2017
…Do` (google#572)

Return rate limit information immediately after checkRateLimitBeforeDo

Fixes google#571.
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

No branches or pull requests

2 participants