Skip to content
This repository was archived by the owner on Nov 7, 2022. It is now read-only.

Add support for rate_limit_status endpoint#124

Merged
dghubble merged 1 commit intodghubble:masterfrom
ltoussaint:feature/rate_limit_status
Nov 30, 2018
Merged

Add support for rate_limit_status endpoint#124
dghubble merged 1 commit intodghubble:masterfrom
ltoussaint:feature/rate_limit_status

Conversation

@ltoussaint
Copy link
Copy Markdown
Contributor

Comment thread twitter/rate_limits.go Outdated
}

// RateLimitStatusResponse response of rate limit status
type RateLimitStatusResponse struct {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's say RateLimit. There is a common tendency in PRs to have Response at the end of things, but its nicer to refer to them by objecty names to separate what they are from how they were obtained.

Comment thread twitter/rate_limits.go Outdated

// RateLimitStatusResponse response of rate limit status
type RateLimitStatusResponse struct {
RateLimitContext *RateLimitContext `json:"rate_limit_context,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

These types aren't ever sent / marshaled to JSON so there should be no need for the omitempty on fields.

Comment thread twitter/rate_limits.go Outdated
}
}

// RateLimitStatusResponse response of rate limit status
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

RateLimitStatus summarizes current rate limits of resource families.

Comment thread twitter/rate_limits.go Outdated

// Status returns rate limits.
// https://developer.twitter.com/en/docs/developer-utilities/rate-limit-status/api-reference/get-application-rate_limit_status
func (s *RateLimitService) Status(params *RateLimitStatusParams) (RateLimitStatusResponse, *http.Response, error) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Returned type should be a pointer for consistency with the rest of the library

Comment thread twitter/rate_limits_test.go Outdated
client := NewClient(httpClient)
tweets, _, err := client.RateLimits.Status(&RateLimitStatusParams{Resources: "statuses,users,help"})
expected := RateLimitStatusResponse{
RateLimitContext: &RateLimitContext{AccessToken: "786491-24zE39NUezJ8UTmOGOtLhgyLgCkPyY4dAcx6NA6sDKw"},
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please make sure this isn't real.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread twitter/rate_limits.go Outdated

// RateLimitStatusParams are the parameters for RateLimitService.Status.
type RateLimitStatusParams struct {
Resources string `url:"resources,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

If the endpoint takes a list of resource kinds, the API should take a slice of those resource kinds. What if a user wanted to construct that list in a higher level function and append items to it conditionally? Let's not assume its one comma separated string please

Comment thread twitter/rate_limits_test.go Outdated
Reset: 1403602426}}}}

assert.Nil(t, err)
assert.Equal(t, expected, tweets)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

tweets?

Comment thread twitter/rate_limits.go Outdated
type RateLimitResource struct {
Limit int `json:"limit,omitempty"`
Remaining int `json:"remaining,omitempty"`
Reset int `json:"reset,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

int64, this is an epoch time. Don't want bugs in 2038.

Comment thread twitter/rate_limits.go Outdated
Users map[string]*RateLimitResource `json:"users,omitempty"`
Statuses map[string]*RateLimitResource `json:"statuses,omitempty"`
Help map[string]*RateLimitResource `json:"help,omitempty"`
Search map[string]*RateLimitResource `json:"search,omitempty"`
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I see these are the 4 keys from the Twitter examples. Have you tried others to try to find the full set or resources?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Comment thread twitter/rate_limits.go Outdated
Resources string `url:"resources,omitempty"`
}

// Status returns rate limits.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's improve this so its more helpful. Status summarizes the current rate limits of specified resource families.

@dghubble
Copy link
Copy Markdown
Owner

Looks pretty good, should be fairly quick fixes.

@ltoussaint ltoussaint force-pushed the feature/rate_limit_status branch from bd5f167 to 3af734a Compare November 28, 2018 12:27
@ltoussaint ltoussaint force-pushed the feature/rate_limit_status branch from 3af734a to 3d48534 Compare November 28, 2018 12:41
Copy link
Copy Markdown
Owner

@dghubble dghubble left a comment

Choose a reason for hiding this comment

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

Thanks!

@dghubble dghubble merged commit 22c4b7b into dghubble:master Nov 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants