Return rate limit information immediately after checkRateLimitBeforeDo#572
Return rate limit information immediately after checkRateLimitBeforeDo#572gmlewis merged 4 commits intogoogle:masterfrom
checkRateLimitBeforeDo#572Conversation
Fixes google#571. Change-Id: Iefcc687d9fc56c40964d5e6f1cdbc54d020dae3c
Change-Id: I09a8c2f3c841fb5f95539db7d2fb0b7e22e4956e
dmitshur
left a comment
dmitshur
left a comment
There was a problem hiding this comment.
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.
I'm not against it, but I want to think it over.
Usually, having 2 ways of doing the same thing is worse than just one canonical way. But in this case, simply returning a *Response that contains Rate seems consistent with what happens in many other situations (including a rate limit error that happens from a real remote GitHub API response).
I had one potential concern about a change in this PR, I've left an inline comment here. Take a look.
Overall, I'm more in favor of doing this. But let's discuss the above first and come to a conclusion.
| requests since then. You can always call RateLimits() directly to get the most | ||
| up-to-date rate limit data for the client. | ||
| You can always call RateLimits() directly to get the most up-to-date | ||
| rate limit data for the client. |
There was a problem hiding this comment.
In addition to removing the outdated part about Rate method on github client, I think we should put in its place a mention of the best way to access the remaining rate limit, and that is by accessing the resp.Rate of the previous call.
That used to be already written as part of the removed Rate method:
Use the Response.Rate returned from most recent API call.
So, how about something like this:
The returned Response.Rate value contains the rate limit information from the most recent API call. If a recent enough response isn't available, you can use RateLimits to fetch the most up-to-date rate limit data for the client.
There was a problem hiding this comment.
I like that. I'll hopefully make these changes early next week.
| if err := c.checkRateLimitBeforeDo(req, rateLimitCategory); err != nil { | ||
| return nil, err | ||
| return &Response{ | ||
| Response: err.Response, |
There was a problem hiding this comment.
I have one potential concern about this change.
I think it's somewhat unfortunate that we ended up being forced to create a fake/mock *http.Response even in situations where no remote HTTP request was actually made. I am referring to this.
It was originally done in #347 because we (or I) didn't want to break clients that assumed *http.Response was never nil, since it used to never be nil in prior to that change.
So I think it's somewhat disadvantageous that we would now be propagating the fake *http.Response in more situations.
However, even if we had control over this, I'm not sure what's better. If a remote API request is not made (because we've predicted it'll fail due to exceeded rate limit), should the client be able to tell it apart from when a remote API request was made? It wasn't documented in the past, but one could've done that by checking if resp was nil. This changes that.
I'm pointing out the above because I wanted to think it through. But I don't think it's a reason to avoid going with this change. This change actually increases consistency (in that we return resp and resp.Rate in more cases).
So, I think I'm okay with doing this, but I want to take another day to think it over. Meanwhile, I'd love to hear your perspective on the above @gmlewis.
There was a problem hiding this comment.
You make excellent points and I have mixed feelings too. Please know that I am completely comfortable with the decision to simply revert the code changes and keep the updates to the documentation.
There is no urgency here so we can let this sit for some time in case others have opinions.
|
I've thought about this and I'm in favor of making this change. Given we already do have a fake response, I think this change simplifies things/adds consistency by returning the fake response in more places, just like a real response. The subtle difference I outlined in #572 (comment), where a really determined individual could tell apart a fake response (no network call made) vs real response (from a network call that was made), is not documented, and cannot really be relied on. So, I'm okay with doing this because it simplifies/streamlines things. I don't think it's going to make a practical difference for most people, given what we've discussed in #571 (comment). But I wanted to make a decision on this PR to move forward, and I think merging it makes more sense than not. However, to merge this, @gmlewis you still need to address https://github.com/google/go-github/pull/572/files#r104281339. |
Change-Id: I1fbe502f4a4001fa2769483af01c8c11721c8aae
|
Thanks, @shurcooL. I believe I have addressed the review feedback, but please let me know if I missed something. |
|
The change to |
Change-Id: I1d66ec282fa35905c13982bdbcd94d78413465aa
|
Ah, thank you, @shurcooL! |
|
Friendly ping @gmlewis. Now that this has my approval, I'm leaving this for you to merge, since you authored this PR. |
…Do` (google#572) Return rate limit information immediately after checkRateLimitBeforeDo Fixes google#571.
2 participants
Fixes #571.
Change-Id: Iefcc687d9fc56c40964d5e6f1cdbc54d020dae3c