Skip to content

(finally!) review readme.md getting started#523

Merged
thinkingserious merged 3 commits intomasterfrom
ksigler7-patch-1
Sep 20, 2017
Merged

(finally!) review readme.md getting started#523
thinkingserious merged 3 commits intomasterfrom
ksigler7-patch-1

Conversation

@apigirl
Copy link
Copy Markdown

@apigirl apigirl commented Sep 20, 2017

  • made small phrasing changes
  • updated API docs link

Also, see my comment for two sections that I think could use clarification.

- made small phrasing changes
- updated API docs link
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Sep 20, 2017
@SendGridDX
Copy link
Copy Markdown

SendGridDX commented Sep 20, 2017

CLA assistant check
All committers have signed the CLA.

README.md Outdated
## Setup Environment Variables

Update the development Environment with your [SENDGRID_API_KEY](https://app.sendgrid.com/settings/api_keys). For example, in Windows 10, please review [this thread](http://superuser.com/questions/949560/how-do-i-set-system-environment-variables-in-windows-10).
Set your [SENDGRID_API_KEY](https://app.sendgrid.com/settings/api_keys) environment variable in your development environment. For example,[this thread](http://superuser.com/questions/949560/how-do-i-set-system-environment-variables-in-windows-10) explains how to set an environment variable in a Windows 10 system.
Copy link
Copy Markdown
Author

@apigirl apigirl Sep 20, 2017

Choose a reason for hiding this comment

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

I think this could be a little more clear still - when you set an environment variable, don't you have to name the variable? Or are we saying that you should add your API key to your PATH? Either way, I think we could make it a bit more clear.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about:

We advise against hard coding your SENDGRID_API_KEY in your application. Instead, please use an environment variable or Web.config or similar.

@apigirl
Copy link
Copy Markdown
Author

apigirl commented Sep 20, 2017

Line 65 is: "Once you have the SendGrid libraries properly referenced in your project, you can include calls..."

What do they have to do to reference them in their project?

@apigirl apigirl changed the title review readme.md getting started (finally!) review readme.md getting started Sep 20, 2017
README.md Outdated
## Setup Environment Variables

Update the development Environment with your [SENDGRID_API_KEY](https://app.sendgrid.com/settings/api_keys). For example, in Windows 10, please review [this thread](http://superuser.com/questions/949560/how-do-i-set-system-environment-variables-in-windows-10).
Set your [SENDGRID_API_KEY](https://app.sendgrid.com/settings/api_keys) environment variable in your development environment. For example,[this thread](http://superuser.com/questions/949560/how-do-i-set-system-environment-variables-in-windows-10) explains how to set an environment variable in a Windows 10 system.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about:

We advise against hard coding your SENDGRID_API_KEY in your application. Instead, please use an environment variable or Web.config or similar.

@apigirl
Copy link
Copy Markdown
Author

apigirl commented Sep 20, 2017

@thinkingserious okay, I updated the Environment Variables section. Thanks for your suggestion! Any thoughts on my other comment?

@thinkingserious
Copy link
Copy Markdown
Contributor

"Once you have the SendGrid libraries properly referenced in your project, you can include calls..." could be "Once you have the SendGrid libraries properly installed in your project, you can include calls..."

@apigirl
Copy link
Copy Markdown
Author

apigirl commented Sep 20, 2017

Is there anything that users have to do to install and reference the project other than what's in the Install Package section?

@thinkingserious
Copy link
Copy Markdown
Contributor

No

@apigirl
Copy link
Copy Markdown
Author

apigirl commented Sep 20, 2017

okay, cool! that makes more sense then. I updated it - let me know if my update is accurate! Other than that, I didn't have anything else!

@thinkingserious thinkingserious merged commit 670ab60 into master Sep 20, 2017
@childish-sambino childish-sambino deleted the ksigler7-patch-1 branch January 16, 2020 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: code review request requesting a community code review or review from Twilio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants