-
Notifications
You must be signed in to change notification settings - Fork 192
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
Fixes #827. Refactor how we request data from GitHub. #828
Conversation
Oops @ |
Part of this means we're always sending request headers to GitHub and always returning response headers from GitHub. Before it was only in some places. This might break things, or it might make them better!
ea5a0a6
to
e655005
Compare
issues.status_code, get_headers(issues)) | ||
# issues is a tuple of format: (content, status_code, response_headers) | ||
if issues[1] == 200: | ||
return (filter_new(json.loads(issues[0])), issues[1], issues[2]) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Also, update some of the comments in api_request helper method.
This already happens in api_request.
OK, I think I addressed your comments here @karlcow -- care to take a peek? |
'content': filter_new(json.loads(issues[0])), | ||
'status_code': issues[1], | ||
'response_headers': issues[2] | ||
} |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
ok. :) Let's move forward. It will help us to have a clearer code. Thanks a lot mike. |
Fixes #827. Refactor how we request data from GitHub.
Thanks @karlcow! |
This should pass all tests (at least it does locally!)
I'm going to do more manual testing and inspection of requests/responses, but it should be ready for review.
r? @karlcow