Skip to content

Conversation

jmctwilio
Copy link
Contributor

No description provided.

@jmctwilio jmctwilio requested review from dougblack and codejudas July 31, 2017 20:32
Copy link
Contributor

@dougblack dougblack left a comment

Choose a reason for hiding this comment

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

Can we add a test or two to assert that this works?

@@ -47,4 +47,7 @@ def request(self, method, url, params=None, data=None, headers=None, auth=None,
timeout=timeout,
)

self.last_request = request
self.last_response = response
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's set these to None in the constructor so developers can always be sure that the attributes exist.

@@ -47,4 +50,7 @@ def request(self, method, url, params=None, data=None, headers=None, auth=None,
timeout=timeout,
)

self.last_request = request
Copy link
Contributor

Choose a reason for hiding this comment

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

can we set this before we send the request? That way if session.send raises an exception it will still be set.

@@ -40,11 +43,15 @@ def request(self, method, url, params=None, data=None, headers=None, auth=None,

request = Request(method.upper(), url, params=params, data=data, headers=headers, auth=auth)

self.last_request = request
Copy link

Choose a reason for hiding this comment

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

Shouldn't these be twilio.Request not requests.models.Request? Same with the response object.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes thats what the other libraries do.

…oper contructions and setting none on request exception
@ghost ghost merged commit b3257ce into master Sep 8, 2017
@ghost ghost deleted the DEVX-5241_add_last_request-response branch September 8, 2017 18:27
Copy link

@wojcikstefan wojcikstefan left a comment

Choose a reason for hiding this comment

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

Hi there, I'm curious about the rationale for adding this feature (something that ideally the PR description would explain). What are example scenarios in which you'd use TwilioHttpClient.last_request/last_response? Alos, should it be documented explicitly that using it is not safe in a threaded environment (see also #580)?

Comment on lines +84 to +85
self.assertIsNotNone(self.client.last_request)
self.assertIsNone(self.client.last_response)

Choose a reason for hiding this comment

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

If the self.client.request(...) call above raises an exception as expected, then these assertions will never run.

This pull request was closed.
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.

4 participants