Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Using transport helper for calling http.request(). #586

Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 3, 2016

This (reference to http.request) assumes (for now) that http is an instance of httplib.Http.

Alternative approach to #583. Towards #128.

Note that this PR stops short of

  • just using http instead of http.request
  • changing the input argument in all helpers from http_request to http

request_method = mock.Mock(
return_value=(response, content.encode('utf-8')))
# Make sure the mock doesn't have a request attr.
del request_method.request

This comment was marked as spam.

@theacodes
Copy link
Contributor

It still seems strange to me that http is an argument to transport.request.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 3, 2016

@jonparrott Baby steps

@theacodes
Copy link
Contributor

@jonparrott Baby steps

But it makes me feel icky.

But more seriously this looks fine so long as there's another PR to nix the http arg.

@theacodes
Copy link
Contributor

theacodes commented Aug 3, 2016

Actually, why not go ahead and make http an optional arg to transport.request?

@dhermes
Copy link
Contributor Author

dhermes commented Aug 3, 2016

I'd rather have a discussion first about your idea of "under the covers" vs. up-front http. Let's hold off on making it optional until we can hash out more details.

transport is essentially complete, except for possibly adding a helper to get status or other headers from a response object.

@@ -1381,6 +1390,10 @@ def _do_retrieve_scopes_test_helper(self, response, content,
assertUrisEqual(self, token_uri, called_uri)
logger.info.assert_called_once_with('Refreshing scopes')

http_request.assert_called_once_with(
token_uri, method='GET', body=None, headers=None,
redirections=5, connection_type=None)

This comment was marked as spam.

@theacodes
Copy link
Contributor

I'd rather have a discussion first about your idea of "under the covers" vs. up-front http. Let's hold off on making it optional until we can hash out more details.

SG.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 3, 2016

@jonparrott Good to merge? @nathanielmanistaatgoogle any thoughts?

@theacodes
Copy link
Contributor

I'm fine with it for now.

@nathanielmanistaatgoogle
Copy link
Contributor

Resolve conflicts and poke me again?

This assumes (for now) that http is an instance of
httplib.Http.
@dhermes
Copy link
Contributor Author

dhermes commented Aug 9, 2016

@nathanielmanistaatgoogle resolved

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants