dev: switch from autopep8 to yapf

autopep8 has a number of issues, including:

- it doesnt handle unicode in files (it ignores encoding)
- its extremely slow
- its code rewriting is based on regexp and is wrong (thus we had to disable a bunch of rules)

.vscode/settings.json

@@ -35,7 +35,7 @@
     "python.linting.pylintEnabled": false,
     "python.linting.flake8Enabled": true,
 
-    "python.formatting.provider": "autopep8",
+    "python.formatting.provider": "yapf",
     "python.pythonPath": "${env.WORKON_HOME}/sentry/bin/python",
     // test discovery is sluggish and the UI around running
     // tests is often in your way and misclicked

setup.cfg

@@ -14,14 +14,14 @@ exclude = .git,*/south_migrations/*,node_modules/*,src/sentry/static/sentry/vend
 [bdist_wheel]
 python-tag = py27
 
-[pep8]
-max-line-length = 99
-# W690 is wrong (e.g. it causes functools.reduce to be imported, which is not compat with Python 3)
-# E700 isnt that important
-# E701 isnt that important
-# E711 could be incorrect
-# E712 could be incorrect
-# E721 says "always us isinstance" which is not a substitute for type in all cases
-ignore = W690,E701,E70,E711,E721
-aggressive = 1
-exclude = */south_migrations/*
+[yapf]
+based_on_style = pep8
+blank_line_before_nested_class_or_def = false
+blank_line_before_class_docstring = false
+coalesce_brackets = false
+column_limit = 100
+dedent_closing_brackets = true
+each_dict_entry_on_separate_line = true
+indent_dictionary_value = false
+split_before_dict_set_generator = false
+split_arguments_when_comma_terminated = false

setup.py

@@ -37,15 +37,12 @@ from setuptools import setup, find_packages
 from setuptools.command.sdist import sdist as SDistCommand
 from setuptools.command.develop import develop as DevelopCommand
 
-ROOT = os.path.realpath(os.path.join(os.path.dirname(
-    sys.modules['__main__'].__file__)))
+ROOT = os.path.realpath(os.path.join(os.path.dirname(sys.modules['__main__'].__file__)))
 
 # Add Sentry to path so we can import distutils
 sys.path.insert(0, os.path.join(ROOT, 'src'))
 
-from sentry.utils.distutils import (
-    BuildAssetsCommand, BuildIntegrationDocsCommand
-)
+from sentry.utils.distutils import (BuildAssetsCommand, BuildIntegrationDocsCommand)
 
 # The version of sentry
 VERSION = '8.19.0.dev0'
@@ -63,11 +60,11 @@ for m in ('multiprocessing', 'billiard'):
 IS_LIGHT_BUILD = os.environ.get('SENTRY_LIGHT_BUILD') == '1'
 
 dev_requires = [
-    'autopep8',
     'Babel',
     'flake8>=2.6,<2.7',
     'pycodestyle>=2.0,<2.1',
     'isort>=4.2.2,<4.3.0',
+    'yapf==0.16.2',
 ]
 
 tests_require = [
@@ -87,7 +84,6 @@ tests_require = [
     'responses',
 ]
 
-
 install_requires = [
     'botocore<1.5.71',
     'boto3>=1.4.1,<1.5',
@@ -110,7 +106,6 @@ install_requires = [
     'honcho>=0.7.0,<0.8.0',
     'kombu==3.0.35',
     'lxml>=3.4.1',
-
     'ipaddress>=1.0.16,<1.1.0',
     'libsourcemap>=0.7.2,<0.8.0',
     'loremipsum>=1.0.5,<1.1.0',
@@ -159,7 +154,6 @@ class SentrySDistCommand(SDistCommand):
 
 
 class SentryBuildCommand(BuildCommand):
-
     def run(self):
         BuildCommand.run(self)
         if not IS_LIGHT_BUILD:
@@ -168,7 +162,6 @@ class SentryBuildCommand(BuildCommand):
 
 
 class SentryDevelopCommand(DevelopCommand):
-
     def run(self):
         DevelopCommand.run(self)
         if not IS_LIGHT_BUILD:
@@ -184,7 +177,6 @@ cmdclass = {
     'build_integration_docs': BuildIntegrationDocsCommand,
 }
 
-
 setup(
     name='sentry',
     version=VERSION,
@@ -209,17 +201,12 @@ setup(
         'console_scripts': [
             'sentry = sentry.runner:main',
         ],
-        'flake8.extension': [
-        ],
+        'flake8.extension': [],
     },
     classifiers=[
-        'Framework :: Django',
-        'Intended Audience :: Developers',
-        'Intended Audience :: System Administrators',
-        'Operating System :: POSIX :: Linux',
-        'Programming Language :: Python :: 2',
-        'Programming Language :: Python :: 2.7',
-        'Programming Language :: Python :: 2 :: Only',
-        'Topic :: Software Development'
+        'Framework :: Django', 'Intended Audience :: Developers',
+        'Intended Audience :: System Administrators', 'Operating System :: POSIX :: Linux',
+        'Programming Language :: Python :: 2', 'Programming Language :: Python :: 2.7',
+        'Programming Language :: Python :: 2 :: Only', 'Topic :: Software Development'
     ],
 )

src/sentry/api/bases/team.py

@@ -17,8 +17,8 @@ class TeamPermission(OrganizationPermission):
     }
 
     def has_object_permission(self, request, view, team):
-        result = super(TeamPermission, self).has_object_permission(
-            request, view, team.organization)
+        result = super(TeamPermission,
+                       self).has_object_permission(request, view, team.organization)
         if not result:
             return result
 
@@ -26,10 +26,7 @@ class TeamPermission(OrganizationPermission):
             return request.auth.organization_id == team.organization.id
 
         allowed_scopes = set(self.scope_map.get(request.method, []))
-        return any(
-            request.access.has_team_scope(team, s)
-            for s in allowed_scopes,
-        )
+        return any(request.access.has_team_scope(team, s) for s in allowed_scopes)
 
 
 class TeamEndpoint(Endpoint):

src/sentry/lint/engine.py

@@ -12,7 +12,6 @@ dependencies such as flake8/pep8.
 """
 from __future__ import absolute_import
 
-
 import os
 import sys
 import subprocess
@@ -44,8 +43,11 @@ def get_files(path):
 
 
 def get_modified_files(path):
-    return [s for s in check_output(
-        ['git', 'diff-index', '--cached', '--name-only', 'HEAD']).split('\n') if s]
+    return [
+        s
+        for s in check_output(['git', 'diff-index', '--cached', '--name-only', 'HEAD'])
+        .split('\n') if s
+    ]
 
 
 def get_files_for_list(file_list):
@@ -65,20 +67,14 @@ def get_files_for_list(file_list):
 def get_js_files(file_list=None):
     if file_list is None:
         file_list = ['tests/js', 'src/sentry/static/sentry/app']
-    return [
-        x for x in get_files_for_list(file_list)
-        if x.endswith(('.js', '.jsx'))
-    ]
+    return [x for x in get_files_for_list(file_list) if x.endswith(('.js', '.jsx'))]
     return file_list
 
 
 def get_python_files(file_list=None):
     if file_list is None:
         file_list = ['src', 'tests']
-    return [
-        x for x in get_files_for_list(file_list)
-        if x.endswith('.py')
-    ]
+    return [x for x in get_files_for_list(file_list) if x.endswith('.py')]
 
 
 def py_lint(file_list):
@@ -93,8 +89,7 @@ def py_lint(file_list):
 
 def js_lint(file_list=None):
 
-    project_root = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir,
-                                os.pardir)
+    project_root = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, os.pardir)
     eslint_path = os.path.join(project_root, 'node_modules', '.bin', 'eslint')
 
     if not os.path.exists(eslint_path):
@@ -107,8 +102,9 @@ def js_lint(file_list=None):
 
     has_errors = False
     if js_file_list:
-        status = Popen([eslint_path, '--config', eslint_config, '--ext', '.jsx', '--fix']
-                       + js_file_list).wait()
+        status = Popen(
+            [eslint_path, '--config', eslint_config, '--ext', '.jsx', '--fix'] + js_file_list
+        ).wait()
         has_errors = status != 0
 
     return has_errors
@@ -129,14 +125,19 @@ def yarn_check(file_list):
         return False
 
     if 'package.json' in file_list and 'yarn.lock' not in file_list:
-        echo(style("""
+        echo(
+            style(
+                """
 Warning: package.json modified without accompanying yarn.lock modifications.
 
 If you updated a dependency/devDependency in package.json, you must run `yarn install` to update the lockfile.
 
 To skip this check, run:
 
-$ SKIP_YARN_CHECK=1 git commit [options]""", fg='yellow'))
+$ SKIP_YARN_CHECK=1 git commit [options]""",
+                fg='yellow'
+            )
+        )
         return True
 
     return False
@@ -147,13 +148,14 @@ def js_format(file_list=None):
     We only format JavaScript code as part of this pre-commit hook. It is not part
     of the lint engine.
     """
-    project_root = os.path.join(os.path.dirname(
-        __file__), os.pardir, os.pardir, os.pardir)
-    prettier_path = os.path.join(
-        project_root, 'node_modules', '.bin', 'prettier')
+    project_root = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, os.pardir)
+    prettier_path = os.path.join(project_root, 'node_modules', '.bin', 'prettier')
 
     if not os.path.exists(prettier_path):
-        echo('[sentry.lint] Skipping JavaScript formatting because prettier is not installed.', err=True)
+        echo(
+            '[sentry.lint] Skipping JavaScript formatting because prettier is not installed.',
+            err=True
+        )
         return False
 
     # Get Prettier version from package.json
@@ -161,42 +163,39 @@ def js_format(file_list=None):
     package_json_path = os.path.join(project_root, 'package.json')
     with open(package_json_path) as package_json:
         try:
-            package_version = json.load(package_json)[
-                'devDependencies']['prettier']
+            package_version = json.load(package_json)['devDependencies']['prettier']
         except KeyError:
             echo('!! Prettier missing from package.json', err=True)
             return False
 
-    prettier_version = subprocess.check_output(
-        [prettier_path, '--version']).rstrip()
+    prettier_version = subprocess.check_output([prettier_path, '--version']).rstrip()
     if prettier_version != package_version:
         echo(
-            '[sentry.lint] Prettier is out of date: {} (expected {}). Please run `yarn install`.'.format(
-                prettier_version,
-                package_version),
-            err=True)
+            '[sentry.lint] Prettier is out of date: {} (expected {}). Please run `yarn install`.'.
+            format(prettier_version, package_version),
+            err=True
+        )
         return False
 
     js_file_list = get_js_files(file_list)
-    return run_formatter([prettier_path,
-                          '--write',
-                          '--single-quote',
-                          '--bracket-spacing=false',
-                          '--print-width=90',
-                          '--jsx-bracket-same-line=true'],
-                         js_file_list)
+    return run_formatter(
+        [
+            prettier_path, '--write', '--single-quote', '--bracket-spacing=false',
+            '--print-width=90', '--jsx-bracket-same-line=true'
+        ], js_file_list
+    )
 
 
 def py_format(file_list=None):
     try:
-        __import__('autopep8')
+        __import__('yapf')
     except ImportError:
-        echo('[sentry.lint] Skipping Python autoformat because autopep8 is not installed.', err=True)
+        echo('[sentry.lint] Skipping Python autoformat because yapf is not installed.', err=True)
         return False
 
     py_file_list = get_python_files(file_list)
 
-    return run_formatter(['autopep8', '--in-place', '-j0'], py_file_list)
+    return run_formatter(['yapf', '--in-place', '-p'], py_file_list)
 
 
 def run_formatter(cmd, file_list, prompt_on_changes=True):
@@ -226,10 +225,11 @@ def run_formatter(cmd, file_list, prompt_on_changes=True):
                 secho('Stage this patch and continue? [Y/n] ', bold=True)
                 if fp.readline().strip().lower() != 'y':
                     echo(
-                        '[sentry.lint] Aborted! Changes have been applied but not staged.', err=True)
+                        '[sentry.lint] Aborted! Changes have been applied but not staged.',
+                        err=True
+                    )
                     sys.exit(1)
-        status = subprocess.Popen(
-            ['git', 'update-index', '--add'] + file_list).wait()
+        status = subprocess.Popen(['git', 'update-index', '--add'] + file_list).wait()
         has_errors = status != 0
     return has_errors
 
@@ -239,10 +239,7 @@ def run(file_list=None, format=True, lint=True, js=True, py=True, yarn=True):
     old_sysargv = sys.argv
 
     try:
-        sys.argv = [
-            os.path.join(os.path.dirname(__file__),
-                         os.pardir, os.pardir, os.pardir)
-        ]
+        sys.argv = [os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, os.pardir)]
         results = []
 
         # packages

src/sentry/plugins/sentry_mail/models.py

@@ -51,9 +51,20 @@ class MailPlugin(NotificationPlugin):
             return self.subject_prefix
         return options.get('mail.subject-prefix')
 
-    def _build_message(self, project, subject, template=None, html_template=None,
-                       body=None, reference=None, reply_reference=None, headers=None,
-                       context=None, send_to=None, type=None):
+    def _build_message(
+        self,
+        project,
+        subject,
+        template=None,
+        html_template=None,
+        body=None,
+        reference=None,
+        reply_reference=None,
+        headers=None,
+        context=None,
+        send_to=None,
+        type=None
+    ):
         if send_to is None:
             send_to = self.get_send_to(project)
         if not send_to:
@@ -125,7 +136,8 @@ class MailPlugin(NotificationPlugin):
             'sentry-account-email-unsubscribe-project',
             kwargs={
                 'project_id': project.id,
-            })
+            }
+        )
 
     def notify(self, notification):
         event = notification.event
@@ -142,9 +154,7 @@ class MailPlugin(NotificationPlugin):
 
         rules = []
         for rule in notification.rules:
-            rule_link = reverse('sentry-edit-project-rule', args=[
-                org.slug, project.slug, rule.id
-            ])
+            rule_link = reverse('sentry-edit-project-rule', args=[org.slug, project.slug, rule.id])
             rules.append((rule.label, rule_link))
 
         enhanced_privacy = org.flags.enhanced_privacy
@@ -167,9 +177,7 @@ class MailPlugin(NotificationPlugin):
                 if not body:
                     continue
                 text_body = interface.to_string(event)
-                interface_list.append(
-                    (interface.get_title(), mark_safe(body), text_body)
-                )
+                interface_list.append((interface.get_title(), mark_safe(body), text_body))
 
             context.update({
                 'tags': event.get_tags(),
@@ -217,7 +225,7 @@ class MailPlugin(NotificationPlugin):
             group = six.next(iter(counts))
             record = max(
                 itertools.chain.from_iterable(
-                    groups.get(group, []) for groups in six.itervalues(digest),
+                    groups.get(group, []) for groups in six.itervalues(digest)
                 ),
                 key=lambda record: record.timestamp,
             )
@@ -249,9 +257,11 @@ class MailPlugin(NotificationPlugin):
     def notify_about_activity(self, activity):
         email_cls = emails.get(activity.type)
         if not email_cls:
-            logger.debug('No email associated with activity type `{}`'.format(
-                activity.get_type_display(),
-            ))
+            logger.debug(
+                'No email associated with activity type `{}`'.format(
+                    activity.get_type_display(),
+                )
+            )
             return
 
         email = email_cls(activity)