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 setting X-RateLimit-Precision #1354

Merged

Conversation

Chris-Johnston
Copy link
Collaborator

@Chris-Johnston Chris-Johnston commented Aug 14, 2019

Implements feature described in discord/discord-api-docs#1064

Adds an option to specify RateLimitPrecision in DiscordConfig so that the X-RateLimit-Reset response header can be more precise. This feature is not yet deployed (from what I have found by hitting ratelimits).

discord/discord-api-docs#1069 Adds X-RateLimit-Reset-After, which instead of specifying the timestamp when ratelimits are reset, specifies the amount of time to delay. This comes with a caveat:

network latency will cause you to be less efficient than using X-RateLimit-Reset with a synchronized clock source.

Should we even bother with X-RateLimit-Reset-After?

Also, when converting from the RateLimitPrecision to a string for the header:

RestClient.SetHeader("X-RateLimit-Precision", RateLimitPrecision.ToString().ToLower());

Do we think that .ToString().ToLower() is fine, or should we add an extension or util method to convert RateLimitPrecision into strings?

This changes the way that the X-RateLimit-Request header is parsed, so that it will work with both integer seconds and float values with seconds and milliseconds
Adds the RateLimitPrecision enum, with Second and Millisecond values. (Do we want to use an extension method to convert it into a string, or is ToString().ToLower() fine?)
Adds RateLimitPrecision as a parameter to DiscordRestApiClient, and to DiscordConfig so that it can set the X-RateLimit-Precision header.
@Joe4evr
Copy link
Contributor

Joe4evr commented Aug 14, 2019

Do we think that .ToString().ToLower() is fine, or should we add an extension or util method to convert RateLimitPrecision into strings?

If this is used a lot, I'd say to write a util method so the resulting strings are cached.

@@ -71,6 +73,7 @@ internal void SetBaseUrl(string baseUrl)
RestClient.SetHeader("accept", "*/*");
RestClient.SetHeader("user-agent", UserAgent);
RestClient.SetHeader("authorization", GetPrefixedToken(AuthTokenType, AuthToken));
RestClient.SetHeader("X-RateLimit-Precision", RateLimitPrecision.ToString().ToLower());
Copy link
Member

Choose a reason for hiding this comment

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

As this conversion only occurs here at the moment, I am alright with it remaining as ToString().ToLower() - however, if in the future we decide (for some reason) to allow this on a per-request basis, we should definitely move to an extension method, a la RateLimitPrecision.AsHeaderValue()

@AntiTcb
Copy link
Collaborator

AntiTcb commented Aug 14, 2019

Is there any value in not having milliseconds be the default?

@Chris-Johnston
Copy link
Collaborator Author

Is there any value in not having milliseconds be the default?

I figured that since the API uses Seconds as the default, I'll stick to that. I don't have any strong opinion on this, though, so either works.

@foxbot foxbot marked this pull request as ready for review September 8, 2019 15:20
@foxbot foxbot merged commit 9482204 into discord-net:dev Sep 8, 2019
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.

None yet

5 participants