fix: Fix and add flake8 to CI #86

hugovk · 2018-10-20T13:31:46Z

Fixes #49
Fixes #89

Checklist

I have made a material change to the repo (functionality, testing, spelling, grammar)
I have read the [Contribution Guide] and my PR follows them.
I updated my branch with the master branch.
I have added tests that prove my fix is effective or that my feature works
[n/a] I have added necessary documentation about the functionality in the appropriate .md file
[n/a] I have added in line documentation to the code I modified

Short description of what this PR does:

Run flake8 (=pyflakes for bugs and pycodestyle for PEP 8 and style) on the CI to enforce compliance
Fix code to pass flake8
flake8 doesn't support Python 2.6 (just drop it: Drop support for EOL Python 2.6 #83)
Fix a typo in the test_lisence.py filename
Make test_license.py use the unittest class for consistency
In tests, use Python's 'or', not '||'
Replaces and closes stalled chore: Follows PEP8 standard #50.

There some overlap between test_license_year in test/test_license.py and test_license_year in test/__init__.py. Should they be merged somehow?

codecov · 2018-10-20T13:33:01Z

Codecov Report

Merging #86 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   99.32%   99.32%           
=======================================
  Files           2        2           
  Lines         149      149           
=======================================
  Hits          148      148           
  Misses          1        1

Impacted Files	Coverage Δ
smtpapi/__init__.py	`98.71% <ø> (ø)`
test/__init__.py	`100.00% <100.00%> (ø)`

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 b3e29d5...0075307. Read the comment docs.

test/__init__.py

42B · 2018-10-28T00:09:26Z

test/test_license.py

+            "Copyright (c) 2012-%s SendGrid, Inc."
+            % datetime.datetime.now().year,
+            copyright_line,
+        )


Even this test seems to have the closing bracket consistent (on a separate line) unlike above.

What do the maintainers think about running Black on the codebase?

https://github.com/ambv/black

@hugovk

Using black is a great idea and sendgrid would only have to send out a fraction of the t-shirts after all the overlapping PEP8 pull requests were closed haha

42B · 2018-10-28T00:10:53Z

test/test_project.py


    # ./docker-compose.yml or ./docker/docker-compose.yml
    def test_docker_compose(self):
-        self.assertEqual(True, os.path.isfile('./docker-compose.yml') || os.path.isfile('./docker/docker-compose.yml'))
+        self.assertEqual(


Couldn't you close #90 if you also used assertTrue's for these again?

Could do, thought it might help with reviewing to have a single PR #90 to address the single issue #89.

@hugovk

Your logic doesn't quite make sense here. I understand you are trying to send as many pull requests as possible but, if anything, this PR has the advantage of taking care of the long lines which #90 doesn't. If you simply fixed changed them all to assertTrue, this PR would be superior to #90

Smaller PRs are definitely easier to review and are much less prone to merge conflicts (as this PR has right now, as has some other recent big PRs), and I'm not bothered if #90 is closed without merging :)

I've changed all of assertEqual(True, x) to assertTrue(x) here.

@hugovk

Your logic doesn't make sense, you have essentially a duplicate PR that you refuse to close... why?

test/test_project.py

42B · 2018-10-28T00:13:04Z

examples/example.py

@@ -2,47 +2,56 @@
 from __future__ import absolute_import, division, print_function

 import time
+from os import path, sys


I'll admit a personal preference towards using import os in this particular case but the real question is consistency. You don't seem to modify these imports in the other files this PR touches, why only here?

Will check the remaining comments later, thanks for the reviews!

Probably only moved imports in this file as flake8 only complained about these ones.

@hugovk

So, why not fix it to be consistent with everything else?

I don't disagree with the sentiment of having consistency. But the acceptance criteria for #49 is simply that everything passes PEP8. So let's focus on that piece here, and then if folks like, other PRs can be opened to improve the consistency.

42B · 2018-10-28T00:15:30Z

setup.py

@@ -6,7 +6,10 @@

 dir_path = os.path.abspath(os.path.dirname(__file__))
 readme = io.open(os.path.join(dir_path, 'README.rst'), encoding='utf-8').read()
-version = io.open(os.path.join(dir_path, 'VERSION.txt'), encoding='utf-8').read().strip()
+version = io.open(
+    os.path.join(dir_path, 'VERSION.txt'),


As above, no modification of import os. Again, I personally prefer it this way and it seems like having consistency throughout the repo would be better overall.

42B · 2018-10-28T00:16:13Z

smtpapi/__init__.py

@@ -7,14 +7,17 @@
 if os.path.isfile(os.path.join(dir_path, 'VERSION.txt')):


Another file, another import os instead of from os import path.

42B · 2018-10-28T00:20:28Z

smtpapi/__init__.py

@@ -67,14 +70,14 @@ def set_sections(self, value):

    def add_send_each_at(self, time):
        if 'send_each_at' not in self.data:
-          self.data['send_each_at'] = []
+            self.data['send_each_at'] = []
        self.data['send_each_at'].append(time)


This function seems like a perfect candidate to change to EAFP over LBYL. Maybe a bit out of scope for this particular PR though but there could be enough of these for a separate PR altogether that could help put you in the top leaderboard spot!

def add_send_each_at(self, time): try: self.data['send_each_at'].append(time) except KeyError: self.data['send_each_at'] = [time]

Yeah, I think that would belong in another PR but I'll leave that for someone else to do.

42B · 2018-10-28T14:04:08Z

test/__init__.py

@@ -128,7 +129,8 @@ def test_repository_files_exists(self):
                self.assertTrue(
                    any(os.path.exists(f) for f in file_path),
                    msg=self.file_not_found_message.format(
-                        '" or "'.join(file_path)),
+                        '" or "'.join(file_path)
+                    ),


Is the trailing comma really necessary?

No, but in general, if another parameter was added (in practice, not likely with assertTrue), it would mean this previous line wouldn't be included in the diff. Happy to remove it if needed.

This is something Black could sort out automatically :)

As I said before, using black would get rid of a bunch of overlapping PRs in sendgrid's repos.

test/test_project.py

hugovk · 2018-10-28T17:54:02Z

Rebased on master to resolve merge conflicts.

42B · 2018-10-28T17:56:14Z

@hugovk

Why haven't you closed #90 yet? This PR has the same changes included plus it seems to have the advantage of breaking up those longer lines.

hugovk · 2018-10-28T18:04:21Z

See #86 (comment).

42B · 2018-10-28T18:08:17Z

@hugovk

If your comment was actually true that you think smaller PRs are better (hard to argue!), why would you make the same changes in this PR which make it bigger? Wouldn't that mean you would have to revert the changes in this PR to make it smaller?

misterdorm · 2018-10-31T16:20:41Z

I'm closing #90, and let's go forward with getting #86 reviewed and merged.

misterdorm

@hugovk Please remove the trailing commas referenced by @42B and me. Any other changes for consistency, etc., I think should be addressed in separate PRs. Please feel free to raise issues against this repo to address those things.

test/test_project.py

misterdorm · 2018-10-31T16:28:55Z

examples/example.py

@@ -2,47 +2,56 @@
 from __future__ import absolute_import, division, print_function

 import time
+from os import path, sys


I don't disagree with the sentiment of having consistency. But the acceptance criteria for #49 is simply that everything passes PEP8. So let's focus on that piece here, and then if folks like, other PRs can be opened to improve the consistency.

hugovk · 2018-10-31T18:56:38Z

Trailing commas removed!

hugovk · 2019-08-07T14:04:15Z

Hello, is there anything extra I can do for this PR? Thanks!

hugovk · 2019-09-29T18:07:42Z

It's nearly time for the next Hacktoberfest! This PR has been approved, is anything else needed to merge it? Thanks!

hugovk · 2019-11-16T10:20:58Z

Closing these year-old PRs. Let me know if you're still interested and I can re-open. Thanks!

thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 20, 2018

42B reviewed Oct 28, 2018

View reviewed changes

hugovk force-pushed the flake8 branch 3 times, most recently from 85000c1 to c762be6 Compare October 28, 2018 17:53

hugovk added 16 commits October 31, 2018 09:05

Fix filename

3d65105

Remove unused imports

e9f58e3

Remove unused variable

749533c

Fix unittest subclass

f5af067

Sort

42c2b56

Ignore .coverage dir

540cc25

Enforce flake8 (=pycodestyle+pyflakes) on CI

31e0b3c

Use Python's 'or', not '||'

8c21060

Ignore the build dir

4e54678

Also exclude defaults

9ed1591

Fix flake8 warnings

cc1f446

Flake8 doesn't support Python 2.6

ab23c27

Tidy imports

d82297c

Consistent newlines

4161601

Use assertTrue(x) over assertEqual(True, x)

f221cb8

Fix rebase

6a7bc47

hugovk force-pushed the flake8 branch from 4974e7e to 6a7bc47 Compare October 31, 2018 07:10

Fix flake8

c897224

misterdorm added difficulty: hard fix is hard in difficulty difficulty: medium fix is medium in difficulty and removed status: code review request requesting a community code review or review from Twilio difficulty: hard fix is hard in difficulty labels Oct 31, 2018

misterdorm mentioned this pull request Oct 31, 2018

Use Python's 'or', not '||' #90

Closed

6 tasks

misterdorm requested changes Oct 31, 2018

View reviewed changes

misterdorm added the type: community enhancement feature request not on Twilio's roadmap label Oct 31, 2018

Remove trailing commas

843018a

misterdorm approved these changes Oct 31, 2018

View reviewed changes

thinkingserious added the status: hacktoberfest approved label Nov 2, 2018

hugovk closed this Nov 16, 2019

childish-sambino reopened this Jul 1, 2020

Merge branch 'master' into flake8

daeaae7

childish-sambino approved these changes Jul 1, 2020

View reviewed changes

childish-sambino changed the title ~~Fix and add flake8 to CI~~ fix: Fix and add flake8 to CI Jul 1, 2020

childish-sambino added 9 commits July 1, 2020 14:46

Update test_project.py

28462d2

Delete test_license.py

51606bf

Update test_project.py

5d1286a

Update test_project.py

27508d9

Update .travis.yml

5f9b7e2

Update requirements.txt

13027c3

Update .flake8

1a055fe

Update __init__.py

3eed609

Update .flake8

0075307

childish-sambino merged commit 3666d43 into sendgrid:master Jul 1, 2020

hugovk deleted the flake8 branch July 2, 2020 05:27

		@@ -7,14 +7,17 @@
		if os.path.isfile(os.path.join(dir_path, 'VERSION.txt')):

fix: Fix and add flake8 to CI #86

fix: Fix and add flake8 to CI #86

Uh oh!

Conversation

hugovk commented Oct 20, 2018 • edited by childish-sambino Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Short description of what this PR does:

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

hugovk commented Oct 28, 2018

Uh oh!

42B commented Oct 28, 2018

Uh oh!

hugovk commented Oct 28, 2018

Uh oh!

42B commented Oct 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

misterdorm commented Oct 31, 2018

Uh oh!

misterdorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugovk commented Oct 31, 2018

Uh oh!

hugovk commented Aug 7, 2019

hugovk commented Oct 20, 2018 •

edited by childish-sambino

Loading

codecov bot commented Oct 20, 2018 •

edited

Loading

42B commented Oct 28, 2018 •

edited

Loading