-
Notifications
You must be signed in to change notification settings - Fork 52
fix: Fix and add flake8 to CI #86
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
Changes from 17 commits
3d65105
e9f58e3
749533c
f5af067
42c2b56
540cc25
31e0b3c
8c21060
4e54678
9ed1591
cc1f446
ab23c27
d82297c
4161601
f221cb8
6a7bc47
c897224
843018a
daeaae7
28462d2
51606bf
5d1286a
27508d9
5f9b7e2
13027c3
1a055fe
3eed609
0075307
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [flake8] | ||
| exclude = build,.svn,CVS,.bzr,.hg,.git,__pycache__,.tox,.eggs,*.egg |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,14 @@ | ||
| *.pyc | ||
| *.egg | ||
| *.egg-info | ||
| dist | ||
| eggs | ||
| build | ||
| sdist | ||
| *.pyc | ||
| .coverage | ||
| .env | ||
| .Python | ||
| bin/ | ||
| build | ||
| dist | ||
| eggs | ||
| include/ | ||
| lib/ | ||
| .env | ||
| sdist | ||
| smtpapi/VERSION.txt |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,24 +3,24 @@ | |
| from smtpapi import SMTPAPIHeader | ||
|
|
||
| import time | ||
| from os import path, sys | ||
|
|
||
| if __name__ == '__main__' and __package__ is None: | ||
| from os import sys, path | ||
|
|
||
| sys.path.append(path.dirname(path.dirname(path.abspath(__file__)))) | ||
| from smtpapi import SMTPAPIHeader | ||
|
|
||
| header = SMTPAPIHeader() | ||
|
|
||
| # [To](http://sendgrid.com/docs/API_Reference/SMTP_API/index.html) | ||
| # header.add_to('[email protected]') | ||
| header.set_tos(['[email protected]', '[email protected]']) | ||
|
|
||
| # [Substitutions](http://sendgrid.com/docs/API_Reference/SMTP_API/substitution_tags.html) | ||
| # [Substitutions] | ||
| # (http://sendgrid.com/docs/API_Reference/SMTP_API/substitution_tags.html) | ||
| # header.add_substitution('key', 'value') | ||
| header.set_substitutions({'key': ['value1', 'value2']}) | ||
|
|
||
| # [Unique Arguments](http://sendgrid.com/docs/API_Reference/SMTP_API/unique_arguments.html) | ||
| # [Unique Arguments] | ||
| # (http://sendgrid.com/docs/API_Reference/SMTP_API/unique_arguments.html) | ||
| # header.add_unique_arg('key', 'value') | ||
| header.set_unique_args({'key': 'value'}) | ||
|
|
||
|
|
@@ -32,16 +32,20 @@ | |
| # header.add_section('key', 'section') | ||
| header.set_sections({'key1': 'section1', 'key2': 'section2'}) | ||
|
|
||
| # [Filters](http://sendgrid.com/docs/API_Reference/SMTP_API/apps.html) | ||
| # [Filters] | ||
| # (http://sendgrid.com/docs/API_Reference/SMTP_API/apps.html) | ||
| header.add_filter('filter', 'setting', 'value') | ||
|
|
||
| # [ASM Group ID](https://sendgrid.com/docs/User_Guide/advanced_suppression_manager.html) | ||
| # [ASM Group ID] | ||
| # (https://sendgrid.com/docs/User_Guide/advanced_suppression_manager.html) | ||
| header.set_asm_group_id('value') | ||
|
|
||
| # [IP Pools](https://sendgrid.com/docs/API_Reference/Web_API_v3/IP_Management/ip_pools.html) | ||
| # [IP Pools] | ||
| # (https://sendgrid.com/docs/API_Reference/Web_API_v3/IP_Management/ip_pools.html) | ||
| header.set_ip_pool("testPool") | ||
|
|
||
| # [Scheduling Parameters](https://sendgrid.com/docs/API_Reference/SMTP_API/scheduling_parameters.html) | ||
| # [Scheduling Parameters] | ||
| # (https://sendgrid.com/docs/API_Reference/SMTP_API/scheduling_parameters.html) | ||
| # header.add_send_each_at(unix_timestamp) # must be a unix timestamp | ||
| # header.set_send_each_at([]) # must be a unix timestamp | ||
| header.set_send_at(int(time.time())) # must be a unix timestamp | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, no modification of |
||
| encoding='utf-8', | ||
| ).read().strip() | ||
| copy_file(os.path.join(dir_path, 'VERSION.txt'), | ||
| os.path.join(dir_path, 'smtpapi', 'VERSION.txt'), | ||
| verbose=0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,11 @@ def test_add(self): | |
|
|
||
| def test_set(self): | ||
| header = SMTPAPIHeader() | ||
| header.set_tos(["[email protected]", "[email protected]", "[email protected]"]) | ||
| header.set_tos([ | ||
| "[email protected]", | ||
| "[email protected]", | ||
| "[email protected]", | ||
| ]) | ||
| header.set_substitutions({ | ||
| "subKey": ["subValue"], | ||
| "decimalKey": [decimal.Decimal("1.23456789")] | ||
|
|
@@ -85,7 +89,11 @@ def test_license_year(self): | |
| if line.startswith('Copyright'): | ||
| copyright_line = line.strip() | ||
| break | ||
| self.assertEqual('Copyright (c) 2013-%s SendGrid, Inc.' % datetime.datetime.now().year, copyright_line) | ||
| self.assertEqual( | ||
| 'Copyright (c) 2013-%s SendGrid, Inc.' | ||
| % datetime.datetime.now().year, | ||
| copyright_line | ||
| ) | ||
|
|
||
|
|
||
| class TestRepository(unittest.TestCase): | ||
|
|
@@ -118,10 +126,17 @@ def test_repository_files_exists(self): | |
| for file_path in self.required_files: | ||
| if isinstance(file_path, list): | ||
| # multiple file paths: assert that any one of the files exists | ||
| self.assertTrue(any(os.path.exists(f) for f in file_path), | ||
| msg=self.file_not_found_message.format('" or "'.join(file_path))) | ||
| self.assertTrue( | ||
| any(os.path.exists(f) for f in file_path), | ||
| msg=self.file_not_found_message.format( | ||
| '" or "'.join(file_path) | ||
| ), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the trailing comma really necessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but in general, if another parameter was added (in practice, not likely with This is something Black could sort out automatically :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I said before, using black would get rid of a bunch of overlapping PRs in sendgrid's repos. |
||
| ) | ||
| else: | ||
| self.assertTrue(os.path.exists(file_path), msg=self.file_not_found_message.format(file_path)) | ||
| self.assertTrue( | ||
| os.path.exists(file_path), | ||
| msg=self.file_not_found_message.format(file_path), | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| try: | ||
| import unittest2 as unittest | ||
| except ImportError: | ||
| import unittest | ||
|
|
||
| import datetime | ||
|
|
||
|
|
||
| class LicenseTests(unittest.TestCase): | ||
| def test_license_year(self): | ||
| with open("license.txt", "r") as f: | ||
| copyright_line = f.readline().rstrip() | ||
|
|
||
| self.assertEqual( | ||
| "Copyright (c) 2012-%s SendGrid, Inc." | ||
| % datetime.datetime.now().year, | ||
| copyright_line, | ||
| ) | ||
|
||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll admit a personal preference towards using
import osin 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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check the remaining comments later, thanks for the reviews!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably only moved imports in this file as flake8 only complained about these ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hugovk
So, why not fix it to be consistent with everything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.