Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAdded Twilio Fixes #7 #18
Conversation
TheThirdOne
added some commits
Jun 19, 2015
zachlatta
reviewed
Jun 24, 2015
| @@ -1,6 +1,6 @@ | ||
| function Maestro(){ | ||
| var self = this; | ||
| this.ws = new WebSocket("ws://localhost:1759/baton/connect") | ||
| this.ws = new WebSocket("ws://" + window.location.host + "/baton/connect"); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
reviewed
Jun 24, 2015
| var ( | ||
| smsCallbacks = make([]callback, 0) | ||
| callCallbacks = make([]callback, 0) | ||
| ) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
Jun 24, 2015
Contributor
We've discussed this before, but I think it's important that the application itself is stateless so we can have multiple instances running (even if just for redundancy).
We can focus on that later though.
zachlatta
Jun 24, 2015
Contributor
We've discussed this before, but I think it's important that the application itself is stateless so we can have multiple instances running (even if just for redundancy).
We can focus on that later though.
zachlatta
reviewed
Jun 24, 2015
| data string | ||
| } | ||
| var client = &http.Client{} |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
Jun 24, 2015
Contributor
What if instead of making the HTTP requests directly from this package, we either separated it into a separate package or used an existing Twilio binding (like https://github.com/sfreiberg/gotwilio)?
zachlatta
Jun 24, 2015
Contributor
What if instead of making the HTTP requests directly from this package, we either separated it into a separate package or used an existing Twilio binding (like https://github.com/sfreiberg/gotwilio)?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
TheThirdOne
Jun 25, 2015
Contributor
Why would we want it in a separate package? Its relatively simple. Also, https://github.com/sfreiberg/gotwilio is relatively useless to this project. (Though I make use a few things from it)
TheThirdOne
Jun 25, 2015
Contributor
Why would we want it in a separate package? Its relatively simple. Also, https://github.com/sfreiberg/gotwilio is relatively useless to this project. (Though I make use a few things from it)
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
Jun 25, 2015
Contributor
Separation of concerns. Two things are happening in this package: (1) baton interface for Twilio, (2) abstraction for interacting with the Twilio API.
zachlatta
Jun 25, 2015
Contributor
Separation of concerns. Two things are happening in this package: (1) baton interface for Twilio, (2) abstraction for interacting with the Twilio API.
zachlatta
reviewed
Jun 24, 2015
| return t.recieveCall(newBody, resp) | ||
| default: | ||
| return errors.New("unknown command: " + cmd) | ||
| } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
Jun 24, 2015
Contributor
Would it be cleaner to have a map[string]func(map[string]interface{}, resp chan<- interface{}) and lookup the command in the map rather than a switch statement?
zachlatta
Jun 24, 2015
Contributor
Would it be cleaner to have a map[string]func(map[string]interface{}, resp chan<- interface{}) and lookup the command in the map rather than a switch statement?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
TheThirdOne
Jun 25, 2015
Contributor
I would consider a map to be significantly more messy than the switch.
TheThirdOne
Jun 25, 2015
Contributor
I would consider a map to be significantly more messy than the switch.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
Jun 25, 2015
Contributor
Eh, no point in arguing right now about this. Can improve this later.
zachlatta
Jun 25, 2015
Contributor
Eh, no point in arguing right now about this. Can improve this later.
zachlatta
reviewed
Jun 24, 2015
| func (t Twilio) makeCall(body map[string]interface{}, resp chan<- interface{}) error { | ||
| to := body["to"].(string) | ||
| from := body["from"].(string) | ||
| twiml := body["twiml"].(string) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
TheThirdOne
Jun 25, 2015
Contributor
Bad things. #10, I still need to make a proper way to consume body.
TheThirdOne
Jun 25, 2015
Contributor
Bad things. #10, I still need to make a proper way to consume body.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
zachlatta
reviewed
Jun 24, 2015
View changes
baton/modules/twilio/twilio.go
TheThirdOne
added some commits
Jun 25, 2015
zachlatta
reviewed
Jun 25, 2015
| to := body["to"].(string) | ||
| from := body["from"].(string) | ||
| twiml := body["twiml"].(string) | ||
| form := url.Values{"To": {to}, "From": {from}, "Url": {"http://524b95fe.ngrok.io/baton/webhooks/Twilio/call"}} //temporary |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
TheThirdOne
Jun 25, 2015
Contributor
As mentioned previously and on #16, this will be solved with configuration. The recent change was due to the merge of #16 which changed the path from /webhooks/... to /baton/webhooks/...
TheThirdOne
Jun 25, 2015
Contributor
As mentioned previously and on #16, this will be solved with configuration. The recent change was due to the merge of #16 which changed the path from /webhooks/... to /baton/webhooks/...
zachlatta
reviewed
Jun 25, 2015
View changes
baton/modules/twilio/twilio.go
added a commit
that referenced
this pull request
Jun 29, 2015
TheThirdOne commentedJun 23, 2015
No description provided.