Skip to content
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

Adding in handling for secondary rate limit exceptions #2473

Merged
merged 2 commits into from
Jul 11, 2022

Conversation

JonruAlveus
Copy link
Collaborator

fixes #2466

Copy link
Contributor

@timrogers timrogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this PR together! ❤️

@@ -719,7 +719,7 @@ static Exception GetExceptionForForbidden(IResponse response)
{
string body = response.Body as string ?? "";

if (body.Contains("rate limit exceeded"))
if (body.Contains("rate limit exceeded") || body.Contains("secondary rate limit"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it might be better throw a different kind of exception for a secondary rate limit. What do you think? I'm just thinking that it would make it easier to handle this specific case as a caller, without doing string matching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get where you are coming from, makes sense to me :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d just define a new exception and raise that. It should be a quick change, and then we can get this approved and merged ✅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed it to be it's own exception.

Copy link
Contributor

@timrogers timrogers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Let me just tag in @nickfloyd who is more of a C# expert than me, and then we can get this merged hopefully. Thanks for your contribution ❤️

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for getting this done! ❤️

@nickfloyd nickfloyd merged commit b023602 into octokit:main Jul 11, 2022
Copy link

@Cook299 Cook299 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just learned to answer you I'll take it and run it on server see what it does get back to you.

@@ -180,7 +180,7 @@ public async Task ThrowsApiValidationExceptionFor422Response()
}

[Fact]
public async Task ThrowsRateLimitExceededExceptionForForbidderResponse()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonruAlveus JonruAlveus deleted the Secondary_Rate_Limit branch July 12, 2022 07:27
@nickfloyd
Copy link
Contributor

release_notes: Adds handling for secondary rate limit exceptions from the GitHub REST API

@nickfloyd nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"You have exceeded a secondary rate limit" does not throw RateLimitExceededException
4 participants