Fixing pep8 issues #65

jlindenger · 2017-10-30T06:08:21Z

Fixes #64

Description of the change: I knocked out some quick fixes for your PEP8 issues. There's actually a few more in there than the ones you linked from CodeClimate as well because I just ran flake8 locally using your tox.ini file.

Reason for the change: Conforming to standards for #64

Regarding #43 - Added PULL_REQUEST_TEMPLATE

Add docker files and update README

SendGridDX · 2017-10-30T06:08:37Z

All committers have signed the CLA.

jlindenger · 2017-10-30T06:40:25Z

I'm not sure what I did that caused some of the older commits to show up here since they were already in master. Any idea?

Madhur96 · 2017-10-30T10:16:40Z

Did you try to squash your commits ?

Simplify github PR template

delirious-lettuce · 2017-10-30T16:57:24Z

python_http_client/client.py

+                        ).encode('utf-8')
+                params = (kwargs['query_params']
+                          if 'query_params' in kwargs
+                          else None)


Again, is there any reason not to just use a regular if/else here?

if 'query_params' in kwargs: params = kwargs['query_params'] else: params = None

Or maybe just an if?

params = None if 'query_params' in kwargs: params = kwargs['query_params']

delirious-lettuce · 2017-10-30T17:00:40Z

python_http_client/client.py

-        url = self._build_versioned_url(url) if self._version else self.host + url
+        url = (self._build_versioned_url(url)
+               if self._version
+               else self.host + url)


It seems like the reason someone would use ternary over a regular if/else is that it can fit on one line. In this case, the line exceeds the PEP8 recommendation and now it is back to being multi-line... shouldn't this go back to being a regular if/else?

if self._version: url = self._build_versioned_url(url) else: url = self.host + url

You make an excellent point. This is why friends don't let friends edit ternary operators in the middle of the night... Will fix. Thanks!

@jlindenger ,

haha no problem!

delirious-lettuce · 2017-10-30T17:03:54Z

register.py


 output = pypandoc.convert('README.md', 'rst')
-f = open('README.txt','w+')
+f = open('README.txt', 'w+')


Shouldn't this use a with statement (also for consistency since with is also used 12 lines below)?

with open('README.txt', 'w+') as f: f.write(output)

delirious-lettuce · 2017-10-30T17:06:28Z

register.py

-replacement = '|SendGrid Logo|\n\n.. |SendGrid Logo| image:: https://uiux.s3.amazonaws.com/2016-logos/email-logo%402x.png\n   :target: https://www.sendgrid.com'
-final_text = readme_rst.replace(replace,replacement)
+replace = ('[SendGrid Logo]\n'
+           '(https://uiux.s3.amazonaws.com/2016-logos/email-logo%402x.png)')


Isn't this what multi-line strings are for?

replace = """\ [SendGrid Logo] (https://uiux.s3.amazonaws.com/2016-logos/email-logo%402x.png)"""

delirious-lettuce · 2017-10-30T17:09:50Z

setup.py

@@ -6,12 +6,14 @@
 if os.path.exists('README.txt'):
    long_description = open('README.txt').read()

+
 def getRequires():


PEP8 == snake_case function names

def get_requires():

delirious-lettuce · 2017-10-30T17:15:48Z

register.py

+replacement = ('|SendGrid Logo|\n\n'
+               '.. |SendGrid Logo| image:: '
+               'https://uiux.s3.amazonaws.com/2016-logos/email-logo%402x.png\n'
+               '   :target: https://www.sendgrid.com')


Too bad a multi-line string couldn't be used quite as well here since the url makes that one line 87 chars long (but no newline characters or extra brackets):

replacement = """\ |SendGrid Logo| .. |SendGrid Logo| image:: https://uiux.s3.amazonaws.com/2016-logos/email-logo%402x.png :target: https://www.sendgrid.com"""

I put a backslash in the multi-line comment to shorten the line. Maybe not the cleanest thing in the world, but I think it works better than the first attempt. Thoughts?

@jlindenger ,

Ah of course! Nice one.

Client can be pickled and unpickled

License date range UnitTest

thinkingserious · 2017-11-02T21:07:55Z

@jlindenger

We can’t merge your PR without the CLA being signed.

[ ] Sign the CLA before November 3rd
[ ] Tag @mbernier and @thinkingserious to let us know it is signed

Then we will comment with a link to get your shirt!

jlindenger · 2017-11-03T04:18:32Z

@mbernier @thinkingserious CLA should be good now. Apparently I had one commit that snuck in with my work email, so I needed to fix that. The conflict is probably something that you should resolve right before you merge (just taking out the unused imports in test_unit.py). I think the underlying file is too much of a moving target to keep pulling in and fixing before a merge.

thinkingserious · 2017-11-03T18:02:54Z

Awesome, thanks @jlindenger! If you have not already, you can redeem your swag here. Thanks!

codecov · 2018-05-22T20:05:03Z

Codecov Report

Merging #65 into master will decrease coverage by 0.05%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
- Coverage   92.22%   92.17%   -0.06%     
==========================================
  Files           6        6              
  Lines         296      294       -2     
==========================================
- Hits          273      271       -2     
  Misses         23       23

Impacted Files	Coverage Δ
tests/test_unit.py	`99.21% <100%> (-0.02%)`	⬇️
python_http_client/__init__.py	`100% <100%> (ø)`	⬆️
python_http_client/client.py	`79.2% <66.66%> (ø)`	⬆️

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 70797c3...6aced08. Read the comment docs.

thinkingserious · 2018-05-22T20:42:15Z

Hello @jlindenger,

Thanks again for the PR!

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

Team SendGrid DX

Matt Bernier and others added 5 commits October 28, 2017 12:04

Merge pull request #44 from s0b0lev/improvement/pull-request-template

7d61799

Regarding #43 - Added PULL_REQUEST_TEMPLATE

Merge branch 'master' of https://github.com/sendgrid/python-http-client

3d5c7ad

added docker build

23d8a0f

Merge pull request #50 from NdagiStanley/master

b43db05

Add docker files and update README

Fixing pep8 issues

d72373c

fixing boolean operator position

d25c00a

pushkyn and others added 2 commits October 30, 2017 16:43

simplify github PR template

163fa10

Merge pull request #69 from pushkyn/simplify-github-pr-template

a05d3ba

Simplify github PR template

delirious-lettuce reviewed Oct 30, 2017

View reviewed changes

jlindenger and others added 6 commits October 30, 2017 22:29

pep8 tweaks based on comments

a8fc230

Merge branch 'master' into fix-pep8-issues

04f3275

Merge remote-tracking branch 'upstream/master'

4774bac

Merge remote-tracking branch 'upstream/master'

b1adf2d

missed one of the recommended review fixes

0439c58

Merge pull request #22 from jussih/pickle-client

bda113f

Client can be pickled and unpickled

apigirl added hacktoberfest and removed hacktoberfest labels Oct 31, 2017

Matt Bernier and others added 3 commits October 31, 2017 21:17

Merge pull request #61 from dinosaurfiles/licensedaterange

67dbcdc

License date range UnitTest

Merge remote-tracking branch 'upstream/master'

7341084

Merge branch 'master' into fix-pep8-issues

3eb7f0a

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 1, 2017

jlindenger added 2 commits November 3, 2017 00:02

Merge remote-tracking branch 'upstream/master'

c3c8b84

Merge remote-tracking branch 'upstream/master'

4ed671a

thinkingserious added the difficulty: medium fix is medium in difficulty label Feb 27, 2018

thinkingserious added the type: twilio enhancement feature request on Twilio's roadmap label Feb 27, 2018

thinkingserious added 2 commits May 22, 2018 11:16

Merge branch 'master' into fix-pep8-issues

9c70f78

Fix merge issue

0fe5450

thinkingserious added 2 commits May 22, 2018 13:08

Add missing import

c2c3c6e

More imports

6aced08

thinkingserious merged commit 6aced08 into sendgrid:master May 22, 2018

Fixing pep8 issues #65

Fixing pep8 issues #65

Uh oh!

Conversation

jlindenger commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes #64

Uh oh!

SendGridDX commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlindenger commented Oct 30, 2017

Uh oh!

Madhur96 commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlindenger Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thinkingserious commented Nov 2, 2017

Uh oh!

jlindenger commented Nov 3, 2017

Uh oh!

thinkingserious commented Nov 3, 2017

Uh oh!

codecov bot commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thinkingserious commented May 22, 2018

Uh oh!

Uh oh!

jlindenger commented Oct 30, 2017 •

edited

Loading

SendGridDX commented Oct 30, 2017 •

edited

Loading

jlindenger Oct 31, 2017 •

edited

Loading

codecov bot commented May 22, 2018 •

edited

Loading