dev: add autope8

.vscode/settings.json

@@ -28,11 +28,16 @@
         "editor.formatOnSave": true
     },
 
+    "[python]": {
+        "editor.formatOnSave": true
+    },
+
     "python.linting.pylintEnabled": false,
     "python.linting.flake8Enabled": true,
 
+    "python.formatting.provider": "autopep8",
     "python.pythonPath": "${env.WORKON_HOME}/sentry/bin/python",
     "python.unitTest.pyTestEnabled": true,
     "python.unitTest.unittestEnabled": false,
     "python.unitTest.nosetestsEnabled": false
-}
+}

bin/lint

@@ -7,7 +7,7 @@ import sys
 # This is to avoid needing to have the `sentry` package explicitly installed.
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), os.pardir, 'src'))
 
-from sentry.lint.engine import check_files
+from sentry.lint.engine import run
 
 offset = 1
 js = True
@@ -26,4 +26,4 @@ file_list = sys.argv[offset:]
 if not file_list:
     file_list = None
 
-sys.exit(check_files(file_list, js=js, py=py))
+sys.exit(run(file_list, js=js, py=py, format=False))

config/hooks/pre-commit

@@ -6,8 +6,6 @@ import sys
 
 from glob import glob
 
-from sentry.lint.engine import check_files, js_format, yarn_check
-
 text_type = type(u'')
 
 # git usurbs your bin path for hooks and will always run system python
@@ -18,25 +16,15 @@ if 'VIRTUAL_ENV' in os.environ:
 
 
 def main():
-    from flake8.hooks import run
-
-    gitcmd = "git diff-index --cached --name-only HEAD"
-
-    _, files_modified, _ = run(gitcmd)
+    from sentry.lint.engine import get_modified_files, run
 
     files_modified = [
         text_type(f)
-        for f in files_modified
+        for f in get_modified_files(os.getcwd())
         if os.path.exists(f)
     ]
 
-    # Prettier formatting must take place before linting
-    js_format(files_modified)
-
-    if yarn_check(files_modified):
-        return True
-
-    return check_files(files_modified)
+    return run(files_modified)
 
 
 if __name__ == '__main__':

setup.cfg

@@ -13,3 +13,10 @@ 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)
+# E721 says "always us isinstance" which is not a substitute for type in all cases
+ignore = W690,E721
+aggressive = 2

setup.py

@@ -63,6 +63,7 @@ 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',
@@ -175,6 +176,7 @@ class SentryDevelopCommand(DevelopCommand):
             self.run_command('build_assets')
             self.run_command('build_integration_docs')
 
+
 cmdclass = {
     'sdist': SentrySDistCommand,
     'develop': SentryDevelopCommand,

src/sentry/lint/engine.py

@@ -12,13 +12,14 @@ dependencies such as flake8/pep8.
 """
 from __future__ import absolute_import
 
+
 import os
 import sys
 import subprocess
 import json
 
-from subprocess import Popen
-from click import echo, style
+from subprocess import check_output, Popen
+from click import echo, secho, style
 
 os.environ['PYFLAKES_NODOCTEST'] = '1'
 
@@ -42,6 +43,11 @@ def get_files(path):
     return results
 
 
+def get_modified_files(path):
+    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):
     if file_list is None:
         files_to_check = get_files('.')
@@ -53,36 +59,38 @@ def get_files_for_list(file_list):
                 files_to_check.extend(get_files(path))
             else:
                 files_to_check.append(os.path.abspath(path))
-    return files_to_check
+    return sorted(set(files_to_check))
 
 
-def py_lint(file_list):
-    from flake8.engine import get_style_guide
+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 file_list
+
 
+def get_python_files(file_list=None):
     if file_list is None:
-        file_list = ['src/sentry', 'tests']
-    file_list = get_files_for_list(file_list)
+        file_list = ['src', 'tests']
+    return [
+        x for x in get_files_for_list(file_list)
+        if x.endswith('.py')
+    ]
+
 
-    # remove non-py files and files which no longer exist
-    file_list = [x for x in file_list if x.endswith('.py')]
+def py_lint(file_list):
+    from flake8.engine import get_style_guide
 
+    file_list = get_python_files(file_list)
     flake8_style = get_style_guide(parse_argv=True)
     report = flake8_style.check_files(file_list)
 
     return report.total_errors != 0
 
 
-def get_js_files(file_list=None):
-    if file_list is None:
-        file_list = ['tests/js', 'src/sentry/static/sentry/app']
-    file_list = get_files_for_list(file_list)
-    file_list = [
-        x for x in file_list
-        if x.endswith(('.js', '.jsx'))
-    ]
-    return file_list
-
-
 def js_lint(file_list=None):
 
     project_root = os.path.join(os.path.dirname(__file__), os.pardir, os.pardir,
@@ -139,11 +147,13 @@ 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('!! 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
@@ -151,50 +161,118 @@ 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('!! Prettier is out of date: %s (expected %s). Please run `yarn install`.'
-            % (prettier_version, package_version), err=True)
+        echo(
+            '[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)
+
+
+def py_format(file_list=None):
+    try:
+        __import__('autopep8')
+    except ImportError:
+        echo('[sentry.lint] Skipping Python autoformat because autopep8 is not installed.', err=True)
+        return False
+
+    py_file_list = get_python_files(file_list)
+
+    return run_formatter(['autopep8', '--in-place'], py_file_list)
+
+
+def run_formatter(cmd, file_list, prompt_on_changes=True):
+    if not file_list:
+        return False
 
     has_errors = False
-    if js_file_list:
-        status = subprocess.Popen([prettier_path, '--write', '--single-quote',
-            '--bracket-spacing=false', '--print-width=90', '--jsx-bracket-same-line=true'] +
-            js_file_list
-        ).wait()
-        has_errors = status != 0
 
-        if not has_errors:
-            # Stage modifications by Prettier
-            status = subprocess.Popen(['git', 'update-index', '--add'] + file_list).wait()
-            has_errors = status != 0
+    status = subprocess.Popen(cmd + file_list).wait()
+    has_errors = status != 0
+    if has_errors:
+        return False
 
+    # this is not quite correct, but it at least represents what would be staged
+    output = subprocess.check_output(['git', 'diff'] + file_list)
+    if output:
+        echo('[sentry.lint] applied changes from autoformatting')
+        for line in output.splitlines():
+            if line.startswith('-'):
+                secho(line, fg='red')
+            elif line.startswith('+'):
+                secho(line, fg='green')
+            else:
+                echo(line)
+        if prompt_on_changes:
+            with open('/dev/tty') as fp:
+                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)
+                    sys.exit(1)
+        status = subprocess.Popen(
+            ['git', 'update-index', '--add'] + file_list).wait()
+        has_errors = status != 0
     return has_errors
 
 
-def check_files(file_list=None, js=True, py=True):
+def run(file_list=None, format=True, lint=True, js=True, py=True, yarn=True):
     # pep8.py uses sys.argv to find setup.cfg
     old_sysargv = sys.argv
-    sys.argv = [
-        os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, os.pardir)
-    ]
 
-    linters = []
-    if py:
-        linters.append(py_lint(file_list))
-    if js:
-        linters.append(js_lint(file_list))
+    if file_list is None:
+        file_list = get_files('.')
 
     try:
-        if any(linters):
+        sys.argv = [
+            os.path.join(os.path.dirname(__file__),
+                         os.pardir, os.pardir, os.pardir)
+        ]
+        results = []
+
+        # packages
+        if yarn:
+            results.append(yarn_check(file_list))
+
+        # bail early if a deps failed
+        if any(results):
+            return 1
+
+        if format:
+            if py:
+                results.append(py_format(file_list))
+            if js:
+                results.append(js_format(file_list))
+
+        # bail early if a formatter failed
+        if any(results):
+            return 1
+
+        if lint:
+            if py:
+                results.append(py_lint(file_list))
+            if js:
+                results.append(js_lint(file_list))
+
+        if any(results):
             return 1
         return 0
     finally: