From 42f844ba27f4b860106cabcd6a7dd80ae564d5e0 Mon Sep 17 00:00:00 2001 From: Nick Worden <9059304+nworden@users.noreply.github.com> Date: Mon, 29 Apr 2019 13:29:13 -0400 Subject: [PATCH] Set up flake8 (#662) * Fix Django files for flake8. * A couple other style fixes for flake8. * setup.cfg file for flake8 and pylint * Replace yapf with flake8 in tools/lint. * Include wsgi.py in lint checks. * Use flake8 in all_tests and travis_tests. * Install flake8, don't install yapf. --- .travis.yml | 4 +-- app/tasksmodule/sitemap_ping.py | 2 +- app/views/admin/acls.py | 4 +-- app/views/admin/base.py | 3 ++ app/views/admin/delete_record.py | 7 +++-- app/views/base.py | 4 +-- setup.cfg | 5 ++++ tests/tasks/test_deletion.py | 18 ++++-------- tests/tasks/test_sitemap_ping.py | 4 +-- tests/test_searcher.py | 6 ++-- tests/testutils/data_generator.py | 14 ++++----- tests/views/test_admin_acls.py | 38 +++++++++++-------------- tests/views/test_admin_delete_record.py | 10 ------- tests/views/test_admin_statistics.py | 3 -- tests/views/test_meta_sitemap.py | 1 - tests/views/view_tests_base.py | 4 +-- tools/all_tests | 2 +- tools/lint | 38 ++++++++++++------------- tools/travis_tests | 2 +- 19 files changed, 76 insertions(+), 93 deletions(-) create mode 100644 setup.cfg diff --git a/.travis.yml b/.travis.yml index 647b20e567..de3e3d9ec2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -35,14 +35,14 @@ install: - | pip install \ cssselect \ + flake8 \ lxml \ mock \ modernize \ parameterized \ pillow==4.1.0 \ pylint==1.9.4 \ - pytest \ - yapf + pytest - pip install -r requirements.txt -t app/vendors - npm --prefix ui install diff --git a/app/tasksmodule/sitemap_ping.py b/app/tasksmodule/sitemap_ping.py index 7e462789a3..3bbcde8665 100644 --- a/app/tasksmodule/sitemap_ping.py +++ b/app/tasksmodule/sitemap_ping.py @@ -97,7 +97,7 @@ def _ping_indexer(self, search_engine): if response.status_code == 200: return True else: - #TODO(nworden): Retry or email konbit-personfinder on failure. + # TODO(nworden): Retry or email konbit-personfinder on failure. logging.error('Received %d pinging %s', response.status_code, ping_url) return False diff --git a/app/views/admin/acls.py b/app/views/admin/acls.py index 749df6fcfe..2578d7ea00 100644 --- a/app/views/admin/acls.py +++ b/app/views/admin/acls.py @@ -102,13 +102,13 @@ def post(self, request, *args, **kwargs): self.params.expiration_date, AdminAclsView._EXPIRATION_DATE_FORMAT) # TODO(nworden): add logging for this if (self.params.get('edit_button', '') or - self.params.get('revoke_button', '')): + self.params.get('revoke_button', '')): acl = admin_acls_model.AdminPermission.get( self.env.repo, email_address) # You can't edit or revoke the permissions of someone at a higher # level. if self.env.user_admin_permission.compare_level_to( - acl.access_level) < 0: + acl.access_level) < 0: raise django.core.exceptions.PermissionDenied if self.params.get('edit_button', ''): acl.access_level = level diff --git a/app/views/admin/base.py b/app/views/admin/base.py index c2c72f45ea..5168747d87 100644 --- a/app/views/admin/base.py +++ b/app/views/admin/base.py @@ -163,6 +163,7 @@ def _enforce_admin_level(user_admin_permission, min_level): if user_admin_permission.compare_level_to(min_level) < 0: raise django.core.exceptions.PermissionDenied + def enforce_moderator_admin_level(func): """Require that the user be a moderator (or return a 403).""" def inner(self, *args, **kwargs): @@ -173,6 +174,7 @@ def inner(self, *args, **kwargs): return func(self, *args, **kwargs) return inner + def enforce_manager_admin_level(func): """Require that the user be a manager (or return a 403).""" def inner(self, *args, **kwargs): @@ -183,6 +185,7 @@ def inner(self, *args, **kwargs): return func(self, *args, **kwargs) return inner + def enforce_superadmin_admin_level(func): """Require that the user be a superadmin (or return a 403).""" def inner(self, *args, **kwargs): diff --git a/app/views/admin/delete_record.py b/app/views/admin/delete_record.py index b6316a1d02..e8a865ff03 100644 --- a/app/views/admin/delete_record.py +++ b/app/views/admin/delete_record.py @@ -55,8 +55,9 @@ def post(self, request, *args, **kwargs): del request, args, kwargs # unused self.enforce_xsrf(self.ACTION_ID) action = ('delete', self.params.id) - path = '/delete?' + utils.urlencode( - {'id': self.params.id, - 'signature': reveal.sign(action),}) + path = '/delete?' + utils.urlencode({ + 'id': self.params.id, + 'signature': reveal.sign(action), + }) return django.shortcuts.redirect( self.build_absolute_uri(path, self.env.repo)) diff --git a/app/views/base.py b/app/views/base.py index 47b8f17564..17234c0217 100644 --- a/app/views/base.py +++ b/app/views/base.py @@ -270,8 +270,8 @@ def render(self, template_name, status_code=200, **template_vars): def get_vars(): """A function returning vars, for use by the resources module.""" template_vars['env'] = self.env - # TODO(nworden): change templates to access config through env, which - # already has the config anyway + # TODO(nworden): change templates to access config through env, + # which already has the config anyway template_vars['config'] = self.env.config template_vars['params'] = self.params template_vars['csp_nonce'] = self.request.csp_nonce diff --git a/setup.cfg b/setup.cfg new file mode 100644 index 0000000000..8ea44e7640 --- /dev/null +++ b/setup.cfg @@ -0,0 +1,5 @@ +[flake8] +max-line-length=80 + +[pylint] +disable=fixme diff --git a/tests/tasks/test_deletion.py b/tests/tasks/test_deletion.py index 0ee9a648dd..1104496549 100644 --- a/tests/tasks/test_deletion.py +++ b/tests/tasks/test_deletion.py @@ -15,24 +15,16 @@ import datetime import logging -import os import sys -import django -import django.test -from google.appengine import runtime -from google.appengine.api import users from google.appengine.ext import db -from google.appengine.api import quota from google.appengine.api import taskqueue -from google.appengine.ext import testbed # We should be moving off mox and to mock. However, taskqueue doesn't play nice # with mock, so dropping mox for these tests means moving to GAE's stuff for # this. Since taskqueue has to get replaced by Cloud Tasks for Python 3 anyway, # it's not worth rewriting these tests now just to avoid mox. import mox -import config import model import utils @@ -136,7 +128,7 @@ def assert_past_due_count(expected): assert db.get(self.key_p1).source_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).entry_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).expiry_date == datetime.datetime(2010, 2, 1) - assert db.get(self.key_p1).is_expired == True + assert db.get(self.key_p1).is_expired is True assert model.Note.get('haiti', self.note_id) is None # Note is hidden assert db.get(self.n1_1.key()) # but the Note entity still exists assert db.get(self.photo_key) @@ -150,7 +142,7 @@ def assert_past_due_count(expected): assert db.get(self.key_p1).source_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).entry_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).expiry_date == datetime.datetime(2010, 2, 1) - assert db.get(self.key_p1).is_expired == True + assert db.get(self.key_p1).is_expired is True assert model.Note.get('haiti', self.note_id) is None # Note is hidden assert db.get(self.n1_1.key()) # but the Note entity still exists assert db.get(self.photo_key) @@ -165,7 +157,7 @@ def assert_past_due_count(expected): assert db.get(self.key_p1).source_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).entry_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).expiry_date == datetime.datetime(2010, 2, 1) - assert db.get(self.key_p1).is_expired == True + assert db.get(self.key_p1).is_expired is True assert db.get(self.key_p1).given_name is None assert model.Note.get('haiti', self.note_id) is None # Note is hidden assert db.get(self.n1_1.key()) is None # Note entity is actually gone @@ -190,12 +182,12 @@ def assert_past_due_count(expected): # Confirm that the task wiped self.p2 as well. assert model.Person.all().count() == 0 assert_past_due_count(2) - assert db.get(self.key_p1).is_expired == True + assert db.get(self.key_p1).is_expired is True assert db.get(self.key_p1).given_name is None assert db.get(self.key_p1).source_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).entry_date == datetime.datetime(2010, 2, 2) assert db.get(self.key_p1).expiry_date == datetime.datetime(2010, 2, 1) - assert db.get(self.key_p2).is_expired == True + assert db.get(self.key_p2).is_expired is True assert db.get(self.key_p2).given_name is None assert db.get(self.key_p2).source_date == datetime.datetime(2010, 3, 15) assert db.get(self.key_p2).entry_date == datetime.datetime(2010, 3, 15) diff --git a/tests/tasks/test_sitemap_ping.py b/tests/tasks/test_sitemap_ping.py index 887b0f0866..a06271112c 100644 --- a/tests/tasks/test_sitemap_ping.py +++ b/tests/tasks/test_sitemap_ping.py @@ -36,8 +36,8 @@ def test_get(self): # pinging anything. config.set(ping_sitemap_indexers=True) with mock.patch('requests.get') as requests_mock: - res = self.run_task('/global/tasks/sitemap_ping', - {'search_engine': 'google'}) + self.run_task( + '/global/tasks/sitemap_ping', {'search_engine': 'google'}) self.assertEqual(len(requests_mock.call_args_list), 1) call_args, _ = requests_mock.call_args_list[0] self.assertEqual(call_args[0], diff --git a/tests/test_searcher.py b/tests/test_searcher.py index f0524b2e7d..2105fafdad 100644 --- a/tests/test_searcher.py +++ b/tests/test_searcher.py @@ -21,7 +21,8 @@ class SearcherTests(unittest.TestCase): def test_full_text_search_results(self): """Use full_text_search.search results when enabled.""" with mock.patch('full_text_search.search') as full_text_search_mock: - full_text_search_mock.return_value = SearcherTests.FULLTEXT_RETURN_VALUE + full_text_search_mock.return_value = ( + SearcherTests.FULLTEXT_RETURN_VALUE) searcher = Searcher( SearcherTests.REPO_NAME, enable_fulltext_search=True, @@ -38,7 +39,8 @@ def test_full_text_results_with_loc(self): """Use full_text_search.search results when enabled, including location. """ with mock.patch('full_text_search.search') as full_text_search_mock: - full_text_search_mock.return_value = SearcherTests.FULLTEXT_RETURN_VALUE + full_text_search_mock.return_value = ( + SearcherTests.FULLTEXT_RETURN_VALUE) searcher = Searcher( SearcherTests.REPO_NAME, enable_fulltext_search=True, diff --git a/tests/testutils/data_generator.py b/tests/testutils/data_generator.py index dd3ee6ce67..24af9f8a21 100644 --- a/tests/testutils/data_generator.py +++ b/tests/testutils/data_generator.py @@ -110,7 +110,7 @@ def note(self, store=True, repo_id='haiti', person_id=None, **kwargs): return note def authorization( - self, store=True, repo_id='haiti', key='secret_key', **kwargs): + self, store=True, repo_id='haiti', key='secret_key', **kwargs): params = copy.deepcopy(TestDataGenerator._DEFAULT_AUTHORIZATION_PARAMS) params.update(kwargs) authorization = model.Authorization.create(repo_id, key, **params) @@ -125,9 +125,9 @@ def photo(self, store=True, repo_id='haiti', image_data='xyz'): return photo def subscription( - self, store=True, repo_id='haiti', person_id=None, - email='fred@example.net', language='en', - timestamp=datetime.datetime(2010, 1, 2)): + self, store=True, repo_id='haiti', person_id=None, + email='fred@example.net', language='en', + timestamp=datetime.datetime(2010, 1, 2)): subscription = model.Subscription( repo=repo_id, person_record_id=person_id, @@ -139,9 +139,9 @@ def subscription( return subscription def admin_permission( - self, store=True, repo_id='haiti', email_address='l@mib.gov', - access_level=admin_acls_model.AdminPermission.AccessLevel.MODERATOR, - expiration_date=datetime.datetime(2011, 1, 20)): + self, store=True, repo_id='haiti', email_address='l@mib.gov', + access_level=admin_acls_model.AdminPermission.AccessLevel.MODERATOR, + expiration_date=datetime.datetime(2011, 1, 20)): admin_permission = admin_acls_model.AdminPermission.create( repo_id, email_address, access_level, expiration_date) if store: diff --git a/tests/views/test_admin_acls.py b/tests/views/test_admin_acls.py index 1ba86d1c02..c7a2b2f763 100644 --- a/tests/views/test_admin_acls.py +++ b/tests/views/test_admin_acls.py @@ -15,12 +15,6 @@ import datetime -import django -import django.http -import django.test - -import config -import model import modelmodule.admin_acls as admin_acls_model import utils @@ -46,12 +40,12 @@ def test_get_with_none_editable(self): """ self.login_as_manager() self.data_generator.admin_permission( - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN) + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN)) self.data_generator.admin_permission( email_address='m@mib.gov', - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN) + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN)) res = self.client.get('/haiti/admin/acls/', secure=True) self.assertEqual(len(res.context['editable_acls']), 0) self.assertEqual(len(res.context['fixed_acls']), 2) @@ -62,8 +56,8 @@ def test_get_with_some_editable(self): self.data_generator.admin_permission() self.data_generator.admin_permission( email_address='m@mib.gov', - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN) + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN)) res = self.client.get('/haiti/admin/acls/', secure=True) self.assertEqual(len(res.context['editable_acls']), 1) self.assertEqual(len(res.context['fixed_acls']), 1) @@ -93,7 +87,7 @@ def test_add_moderator(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect_one('input[name="xsrf_token"]').get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-04-25', @@ -118,7 +112,7 @@ def test_add_manager(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect_one('input[name="xsrf_token"]').get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-04-25', @@ -143,7 +137,7 @@ def test_add_superadmin(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect_one('input[name="xsrf_token"]').get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-04-25', @@ -187,7 +181,7 @@ def test_edit_access_level(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect('input[name="xsrf_token"]')[1].get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-04-25', @@ -208,8 +202,8 @@ def test_managers_cant_edit_superadmins(self): self.data_generator.admin_permission( email_address='l@mib.gov', expiration_date=datetime.datetime(2019, 4, 25), - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN) + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN)) get_doc = self.to_doc(self.client.get( '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect('input[name="xsrf_token"]')[0].get( @@ -252,7 +246,7 @@ def test_edit_expiration_date(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect('input[name="xsrf_token"]')[1].get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-05-25', @@ -275,7 +269,7 @@ def test_revoke_access(self): '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect('input[name="xsrf_token"]')[1].get( 'value') - post_resp = self.client.post('/haiti/admin/acls', { + self.client.post('/haiti/admin/acls', { 'xsrf_token': xsrf_token, 'email_address': 'l@mib.gov', 'expiration_date': '2019-04-25', @@ -290,8 +284,8 @@ def test_managers_cant_revoke_superadmins(self): self.login_as_manager() self.data_generator.admin_permission( email_address='l@mib.gov', - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN) + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN)) get_doc = self.to_doc(self.client.get( '/haiti/admin/acls/', secure=True)) xsrf_token = get_doc.cssselect('input[name="xsrf_token"]')[0].get( diff --git a/tests/views/test_admin_delete_record.py b/tests/views/test_admin_delete_record.py index c34d65a93d..0d64daaef2 100644 --- a/tests/views/test_admin_delete_record.py +++ b/tests/views/test_admin_delete_record.py @@ -13,22 +13,12 @@ # limitations under the License. """Tests for the admin delete-record page.""" -import datetime - import django import django.http import django.test import six.moves.urllib.parse as urlparse -# pylint: disable=wrong-import-order -# pylint sometimes thinks config is a standard import that belongs before the -# django import. It's mistaken; config is our own module (if you run -# python -c "import config", it produces an error saying config doesn't exist). -# Filed issue #626 to move us out of the global namespace someday, which would -# prevent stuff like this. -import config import const -import model import view_tests_base diff --git a/tests/views/test_admin_statistics.py b/tests/views/test_admin_statistics.py index 5f5373ed94..0cff3a30b4 100644 --- a/tests/views/test_admin_statistics.py +++ b/tests/views/test_admin_statistics.py @@ -1,6 +1,3 @@ -import django -import django.test - import model import view_tests_base diff --git a/tests/views/test_meta_sitemap.py b/tests/views/test_meta_sitemap.py index f306c7fbc3..3d5e3e62ff 100644 --- a/tests/views/test_meta_sitemap.py +++ b/tests/views/test_meta_sitemap.py @@ -22,7 +22,6 @@ # Filed issue #626 to move us out of the global namespace someday, which would # prevent stuff like this. import config -import model import view_tests_base diff --git a/tests/views/view_tests_base.py b/tests/views/view_tests_base.py index 50470b984c..55bfe28c70 100644 --- a/tests/views/view_tests_base.py +++ b/tests/views/view_tests_base.py @@ -17,8 +17,8 @@ def setUp(self): self._xsrf_tool = utils.XsrfTool() self.data_generator.admin_permission( repo_id='global', email_address='z@mib.gov', - access_level= - admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN, + access_level=( + admin_acls_model.AdminPermission.AccessLevel.SUPERADMIN), expiration_date=datetime.datetime(2051, 1, 20)) self.data_generator.admin_permission( repo_id='global', email_address='k@mib.gov', diff --git a/tools/all_tests b/tools/all_tests index 06dbea2541..ff089660a9 100755 --- a/tools/all_tests +++ b/tools/all_tests @@ -17,7 +17,7 @@ pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null -$TOOLS_DIR/lint pylint-check && \ +$TOOLS_DIR/lint pylint-check && $TOOLS_DIR/lint flake8-check && \ $TOOLS_DIR/unit_tests -q && \ $TOOLS_DIR/py3_unit_tests -q && \ $TOOLS_DIR/server_tests -q && \ diff --git a/tools/lint b/tools/lint index f7c93ac645..74de754817 100755 --- a/tools/lint +++ b/tools/lint @@ -2,16 +2,19 @@ pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null -# yapf can fix things automatically, but pylint is more thorough. I expect the -# set of things yapf is happy with will grow faster than the set of things -# pylint is happy with, so we separate these lists. -DEFAULT_YAPF_FILES="app/views/admin/base.py \ - tests/test_searcher.py \ - tests/test_xsrftool.py \ - tests/views/test_admin_create_repo.py \ - tests/views/test_auto_security.py \ - tests/views/test_admin_create_repo.py" +# pylint is considerably more thorough than flake8, so we expect the set of +# things flake8 is happy with will grow faster than the set of things pylint is +# happy with, so we separate these lists. +DEFAULT_FLAKE8_FILES="app/views \ + app/tasksmodule \ + app/wsgi.py \ + tests/views \ + tests/tasks \ + tests/testutils \ + tests/test_searcher.py \ + tests/test_xsrftool.py" DEFAULT_PYLINT_FILES="app/views/admin/base.py \ + app/wsgi.py \ tests/test_searcher.py \ tests/test_xsrftool.py \ tests/views/test_admin_create_repo.py \ @@ -19,30 +22,27 @@ DEFAULT_PYLINT_FILES="app/views/admin/base.py \ # Call through python -m instead of directly, so that you're sure to be able to # use it even if you installed the tools through pip. -YAPF="python -m yapf --style google" -# pylint will complain about TODOs, so disable that with "--disable fixme" -PYLINT="python -m pylint --disable fixme" +FLAKE8="python -m flake8" +PYLINT="python -m pylint" command="$1" file_list="${@:2:999}" if [ "$file_list" == "" ]; then - if [ "$command" == "yapf-fix" -o "$command" == "yapf-check" ]; then - file_list=$DEFAULT_YAPF_FILES + if [ "$command" == "flake8-check" ]; then + file_list=$DEFAULT_FLAKE8_FILES elif [ "$command" == "pylint-check" ]; then file_list=$DEFAULT_PYLINT_FILES fi fi -if [ "$command" == "yapf-fix" ]; then - $YAPF -i $file_list -elif [ "$command" == "yapf-check" ]; then - $YAPF -d $file_list +if [ "$command" == "flake8-check" ]; then + $FLAKE8 $file_list elif [ "$command" == "pylint-check" ]; then # Pylint apparently runs some stuff, including App Engine code that expects # this to be set. export SERVER_SOFTWARE="testing" $PYLINT $file_list else - echo "Usage: tools/lint [yapf-fix|yapf-check|pylint-check]" + echo "Usage: tools/lint [flake8-check|pylint-check]" fi diff --git a/tools/travis_tests b/tools/travis_tests index 8f00918826..b5a69b7abc 100755 --- a/tools/travis_tests +++ b/tools/travis_tests @@ -23,7 +23,7 @@ pushd "$(dirname $0)" >/dev/null && source common.sh && popd >/dev/null case "${TRAVIS_PYTHON_VERSION:0:1}" in "2") - $TOOLS_DIR/lint pylint-check && \ + $TOOLS_DIR/lint pylint-check && $TOOLS_DIR/lint flake8-check && \ $TOOLS_DIR/unit_tests -q && \ $TOOLS_DIR/server_tests -q && \ $TOOLS_DIR/python3_compatibility_check && \