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

Add support for the RateLimit-Reset header #9

Merged

Conversation

maxprokopiev
Copy link
Contributor

https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html#rfc.section.3.3

Lots of APIs use a set of 3 RateLimit-xxx headers:

RateLimit-Limit: containing the requests quota in the time window;
RateLimit-Remaining: containing the remaining requests quota in the current window;
RateLimit-Reset: containing the time remaining in the current window, specified in seconds or as a timestamp;

where RateLimit-Reset can easily be greater than Retry-After (no idea why, I guess it's up to the developers of the API).

What do you think about adding support for that header along with Retry-After?

Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

This change looks solid, and useful!

@@ -212,21 +212,16 @@ def rewind_files(body)
end
end

# RFC for RateLimit Header Fields for HTTP:
# https://tools.ietf.org/id/draft-polli-ratelimit-headers-00.html#rfc.section.3.3
Copy link
Member

Choose a reason for hiding this comment

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

Nicely added resource URLs!

@maxprokopiev
Copy link
Contributor Author

maxprokopiev commented Jun 7, 2022

@olleolleolle thank you for the lightning fast review ✨ in df7f0f3 I've also added a small change to the readme

what's the release process of the gem? should I just merge this one and you handle the rest? (update: I clearly can't merge this 😅 since I just have read access)

@olleolleolle
Copy link
Member

@maxprokopiev Features, like this PR, get review, perhaps by multiple readers, then are merged. That's the PR.

Then, a short, mostly automated procedure for getting a release out is done by a maintainer.

If the repository has a CHANGELOG document at all, it'll be a nice thing to create a log line for it, so that the releasing person can use that in descriptions, but if no such document exists, we use other automated tools to make a GitHub Releases "What's Changed?" snippet of text.

Hope this clarifies!

@olleolleolle olleolleolle merged commit 41e5500 into lostisland:main Jun 7, 2022
@maxprokopiev maxprokopiev deleted the rate-limit-reset-header-support branch June 7, 2022 18:54
@maxprokopiev
Copy link
Contributor Author

good to know, @olleolleolle thanks for the explanation and for merging this!

@olleolleolle
Copy link
Member

@maxprokopiev The main branch will be cut as 2.0.0, I think, since it contains breaking changes (keyword arguments-in-middleware-related, see CHANGELOG.md). Investigating!

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

Successfully merging this pull request may close these issues.

2 participants