Skip to content
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

Create Calls resource #4

Merged
merged 25 commits into from
Apr 1, 2015
Merged

Conversation

rudigiesler
Copy link

Create the Calls resource which allows creating calls. Only create the POST root method, as this is the only method that will be used.

@rudigiesler
Copy link
Author

@rudigiesler
Copy link
Author

This PR is ready for review. You will notice that while it makes the initial call, it doesn't do the next step of fetching the TwiML from the supplied URL when that call connects/fails, and it doesn't call the supplied URL when the call ends. This will all be done in #5 , I felt that this PR was getting too big and needed to be split out.

return str(uuid.uuid4()).replace('-', '')

def _get_timestamp(self):
return datetime.now(tzlocal()).strftime('%a, %d %b %Y %H:%M:%S %z')

Choose a reason for hiding this comment

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

Is there a reason to prefer local time over UTC?

Copy link
Author

Choose a reason for hiding this comment

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

No real reason, it's just what I'm used to using. We could very well use tzutc instead. It's what their API seems to be using (+0000)

Copy link

Choose a reason for hiding this comment

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

@hodgestar
Copy link

Left some minor comments. Looks good though.

Thank you for breaking this up and not putting it all in one PR -- very much appreciated. :)

@rudigiesler
Copy link
Author

@hodgestar comments should all be addressed. Ready for another review.

@hodgestar
Copy link

👍

My suggestion for the tests was slightly different to what you implemented. The assertion method is cool, but each test has the same structure:

  • make request to url
  • assert response is an error
    I was suggesting making a method which does both the request and the assert so that the tests become essentially a single function call.

@rudigiesler
Copy link
Author

@hodgestar Isn't that what I implemented? The only function that has more than that is the one which checks for valid non-error responses when one but not the other field is excluded.

rudigiesler pushed a commit that referenced this pull request Apr 1, 2015
@rudigiesler rudigiesler merged commit 8dbbddf into develop Apr 1, 2015
@rudigiesler rudigiesler deleted the feature/issue-4-create-calls-resource branch April 1, 2015 08:15
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