Skip to content

Update Contributing.md - #424

Merged
thinkingserious merged 3 commits into
sendgrid:masterfrom
lafentres:patch-1
May 30, 2018
Merged

Update Contributing.md#424
thinkingserious merged 3 commits into
sendgrid:masterfrom
lafentres:patch-1

Conversation

@lafentres

@lafentres lafentres commented Oct 23, 2017

Copy link
Copy Markdown
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.

@lafentres
Update Contributing.md
31221da 
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

SendGridDX commented Oct 23, 2017 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov-io

codecov-io commented Oct 23, 2017 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

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.

mbernier
mbernier suggested changes Oct 25, 2017
View reviewed changes
Comment thread CONTRIBUTING.md
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

@mbernier mbernier Oct 25, 2017

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.

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

@lafentres lafentres Oct 25, 2017

Copy link
Copy Markdown
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.

@mbernier mbernier Oct 28, 2017

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.

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!!

@lafentres lafentres mentioned this pull request Oct 25, 2017
Merged
@lafentres
Update CONTRIBUTING.md
d8967ae 
Adding clarification to docker vs local install and running python setup.py install command
@lafentres

lafentres commented Nov 5, 2017

Copy link
Copy Markdown
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

thinkingserious commented Nov 9, 2017

Copy link
Copy Markdown
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

thinkingserious commented May 30, 2018

Copy link
Copy Markdown
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

Reviewers

1 more reviewer
@mbernier mbernier mbernier requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lafentres @SendGridDX @codecov-io @thinkingserious @mbernier @apigirl