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

Modified the README.md to include a second example #40

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Modified the README.md to include a second example #40

wants to merge 2 commits into from

Conversation

asparks1805
Copy link

Modified the README.md to include a second example where the account SID and
auth token are embedded in the code rather than stored as environmental
variables. This would allow a developer to compile a self-contained executable
that could be run by end users without the need to create or understand
environment variables. Also modified both examples to define sending and
receiving phone number bindings to make it easier for the developer to
read/configure.


This change is Reviewable

Modified the README.md to include a second example where the account SID and
auth token are embedded in the code rather than stored as environmental
variables. This would allow a developer to compile a self-contained executable
that could be run by end users without the need to create or understand
environment variables. Also modified both examples to define sending and
receiving phone number bindings to make it easier for the developer to
read/configure.
README.md Outdated
let body = PostMessage "+14158059869" "+14158059869" "Oh, hai"
let receivingPhone = "+14158059869"
let sendingPhone = "+14158059869"
let body = PostMessage receivingPhone sendingPhone "Oh, hai"
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about using the names to and from instead of receivingPhone and sendingPhone? This would better match the REST API.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, works for me. I'll make the change and update the pull request tomorrow morning.

Aaron

Cleared the review notes for commit 2afd62a (modified README for a second
example). Modified the following:
* Used to/from on phone bindings to match with Twilio's RESTful API
* Alphabetized the imports
* Used simpler Data.Text (Text) rather than qualified
* Switched to 2 space indent from 4
@StevenXL
Copy link
Collaborator

@markandrus Hi Mark. It seems like this PR is over a year old. Would like to know if you want to update it and try to merge it into master, or if it is a good candidate for closing.

(P.S., haven't actually looked at it, just the date it was opened).

@markandrus
Copy link
Owner

Sorry, I kind of dropped the ball on @asparks1805 PR. Some notes:

  • @asparks1805 identified a useful function, parseCredentials, that we hadn't exported. It'd probably be useful to export this.
  • I don't think we should land the "using SID and secret embedded in the code" example, since this is kind of an anti-pattern (someone could extract the credentials from the binary). Twilio's other examples prefer environment variables here.
  • Otherwise, I do think binding the variable names in the example is useful (e.g., toPhone, etc.)—I get the order confused whenever I re-read it.

I'm also reminded of this suggestion to improve some of our examples.

I don't think we should merge this as-is. Maybe we can run with some of the bullet points above, though?

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.

3 participants