Expose rate limit headers into instance variables#202
Expose rate limit headers into instance variables#202smaeda-ks merged 10 commits intoxdevplatform:masterfrom
Conversation
| return self._account | ||
|
|
||
| def from_response(self, response): | ||
| def from_response(self, response, headers=None): |
There was a problem hiding this comment.
Adding a new argument headers where caller can pass response.headers as an optional argument.
Currently, caller passes only response body:
return self.from_response(response.body['data'])so adding response headers in order to process rate limit headers:
return self.from_response(response.body['data'], response.headers)see tests/test_rate_limit.py for this point.
There was a problem hiding this comment.
Just reminder to follow up about debugging level headers and exposing those as well (X- headers)
There was a problem hiding this comment.
Yup, we can generalize the extract_rate_limit() method in the future to handle more.
There was a problem hiding this comment.
+1 to John's point. I believe there are plans to include rate limit headers to the async stats create endpoint. Given the current setup, this should be fairly trivial to add. Good stuff!
There was a problem hiding this comment.
Thanks! I just changed the func name to extract_response_headers() so this can be consistent for future use such as async one @tushdante mentioned.
| self._raw_body = kwargs.get('raw_body', None) | ||
|
|
||
| if headers.get('content-type') == 'application/gzip': | ||
| # hack because Twitter TON API doesn't return headers as it should |
There was a problem hiding this comment.
Since this is not really correct I'm correcting this. re: PAEX-2015
| if 'x-rate-limit-reset' in headers: | ||
| self._rate_limit = int(headers['x-rate-limit-limit']) | ||
| self._rate_limit_remaining = int(headers['x-rate-limit-remaining']) | ||
| self._rate_limit_reset = datetime.fromtimestamp(int(headers['x-rate-limit-reset'])) |
There was a problem hiding this comment.
I'm fine with not converting raw header values to a specific format like this. We can just leave it as is so that users can process as they want to. Plus, this doesn't necessarily be a breaking change as most of the users weren't able to access these values anyway unless they do a manual request approach.
smaeda-ks
requested review from
jbabichjapan,
jcipriano,
juanshishido and
osowskit
remove duplicate code
|
@tushdante It would be great to get your insights too if possible before I merge this, thanks! |
Yep! I'll take a look at this today! |
|
ping |
tushdante
left a comment
tushdante
left a comment
There was a problem hiding this comment.
Overall, LGTM! Just wanted to get your thoughts on the case where headers aren't returned.
| return self._account | ||
|
|
||
| def from_response(self, response): | ||
| def from_response(self, response, headers=None): |
There was a problem hiding this comment.
+1 to John's point. I believe there are plans to include rate limit headers to the async stats create endpoint. Given the current setup, this should be fairly trivial to add. Good stuff!
| This helper handles all necessary type coercions as it assigns | ||
| attribute values. | ||
| """ | ||
| if headers is not None: |
There was a problem hiding this comment.
What happens for the case where headers is None and the calling function i.e., return self.from_response(response.body['data'], response.headers) has the response.headers set? Ideally this should have something like cursor.account_rate_limit set to None as well.
There was a problem hiding this comment.
@tushdante Thanks! That's a good point... I removed explicit "if" conditions from the extract_response_headers() and changed to use dict.get() method so they get either actual value from response or None in case if it's not present in response, and always accessible through instance variable.
3 participants
re: APE-537
Adding a few lines in
def __from_response()(cursor.py) anddef from_response()(resource.py) to set rate limit instance varialbes.With this change, a caller who invokes
Cursorclass will be able to access rate limit headers through its instance variables. A caller who invokesfrom_response()fromResourceclass will be able to access rate limit headers by passing an additional argument (optional).