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

Update Contributing.md #424

Merged
merged 3 commits into from
May 30, 2018
Merged

Update Contributing.md #424

merged 3 commits into from
May 30, 2018

Conversation

lafentres
Copy link
Contributor

Updating version 2.7.8 to 2.7.11 to match version in pyenv install instruction.

I noticed this when I was setting up my development environment and thought I'd go ahead and update it.

Updating version 2.7.8 to 2.7.11 to match version in pyenv install.
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 23, 2017
@SendGridDX
Copy link

SendGridDX commented Oct 23, 2017

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Oct 23, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@4ae3c96). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #424   +/-   ##
=========================================
  Coverage          ?   81.54%           
=========================================
  Files             ?        9           
  Lines             ?      981           
  Branches          ?      156           
=========================================
  Hits              ?      800           
  Misses            ?       90           
  Partials          ?       91

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ae3c96...05edc63. Read the comment docs.

CONTRIBUTING.md Outdated
@@ -151,7 +151,7 @@ pyenv install 2.7.11
pyenv install 3.4.3
pyenv install 3.5.0
python setup.py install
pyenv local 3.5.0 3.4.3 2.7.8 2.6.9
pyenv local 3.5.0 3.4.3 2.7.11 2.6.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to change this to 2.7.* ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. There are actually some other issues with this file as well. I had a hard time getting set up and ended up just using Docker. I'd be happy to look a little closer at the areas I had issues with and propose some changes, if you think that would be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be absolutely amazing! Please do!
Also, if you had any issues with Docker, please suggest changes there too.

Our theory is that if you had issues, someone else is going to - so any time you see something, please let us know! We would much rather have 1000 PRs fixing small silly issues than 1 customer having trouble using the library.

THANK YOU!!

Adding clarification to docker vs local install and running python setup.py install command
@lafentres
Copy link
Contributor Author

I've added some things I think will make things a little clearer to first-timers getting set up. But I have a few questions:

  1. I never got this command to work:
    source venv/bin/activate
    There's no direct mention of venv anywhere and I just get the following error:
    -bash: venv/bin/activate: No such file or directory. I'm not sure how to resolve this one

  2. I would like to make it a little more clear which steps you still need to run with the Docker image and which you only have to run if you are installing and running locally.
    My initial thoughts are that, with the Docker image, you still need to do the "Environment Variables" section but nothing else (so none of the install/setting up under the "Testing" section). Is that correct?

@thinkingserious
Copy link
Contributor

Thanks for the great feedback @lafentres!

My apologies for the slow response. We are currently working on each PR from oldest to newest as we try to catch up from an amazing Hacktoberfest where we received over 1,000 PRs!

Thank you for your continued support and patience.

With Best Regards,

Elmer

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap difficulty: easy fix is easy in difficulty labels Feb 27, 2018
@thinkingserious thinkingserious merged commit 718ff6a into sendgrid:master May 30, 2018
@thinkingserious
Copy link
Contributor

Hello @lafentres,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy fix is easy in difficulty status: code review request requesting a community code review or review from Twilio type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants