Skip to content

Conversation

petros-p
Copy link

@petros-p petros-p commented Jun 4, 2020

Foursquare API returns a 403 error, and the response object returned by their API will be empty, if user is over their quota limits. They will also include a X-RateLimit-Reset header in the response, which is a timestamp that corresponds to when the rate limits will reset. This info can be found here: https://developer.foursquare.com/docs/places-api/rate-limits/. The proposed changes are a start, there may be additional exception handling that needs to be done to fully implement this change. However, I think the author of this library may know better where changes need to be made. However, for this library to be useful for users with quotas, I think this change is necessary. Feel free to add to, make comments, and build off of this proposal.

Foursquare API returns a 403 error, and the response object returned by their API will be empty, if user is over their quota limits. They will also include a X-RateLimit-Reset header in the response, which is a timestamp that corresponds to when the rate limits will reset. This info can be found here: https://developer.foursquare.com/docs/places-api/rate-limits/. The proposed changes are a start, there may be additional exception handling that needs to be done to fully implement this change. However, I think the author of this library may know better where changes need to be made. However, for this library to be useful for users with quotas, I think this change is necessary. Feel free to add to, make comments, and build off of this proposal.
@mLewisLogic
Copy link
Owner

There are some rate limiting tests available in /foursquare/tests/test_ratelimit.py. Mind adding one for this there?
Steps on how to run tests are here: https://github.com/mLewisLogic/foursquare#testing

return _raise_error_from_response(data)
# Rate limit exceeded, user must wait for quota to reset in order to retrieve data
elif response.status_code == 403:
return {"headers": response.headers, "data": data}
Copy link
Owner

Choose a reason for hiding this comment

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

This seems a bit dangerous. The response structure looks the same as the 200.
We should probably raise an exception instead, with the response.headers included in it.

@petros-p petros-p marked this pull request as draft July 2, 2020 14:55
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