diff --git a/.travis.yml b/.travis.yml index 65c2a65..f35ff8f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,7 +6,6 @@ python: - "3.3" - "3.4" - "3.5" - - "3.6" before_install: - pip install -U pip setuptools install: diff --git a/Makefile b/Makefile index cbef552..f5acace 100644 --- a/Makefile +++ b/Makefile @@ -27,7 +27,7 @@ autolint: autopep8 lint run_tests: clean py.test --durations=10 . -test: autopep8 lint run_tests +test: autopep8 run_tests lint setup_dev: pip install -e .[dev] diff --git a/pylintrc b/pylintrc index 6ce2703..8020e7a 100644 --- a/pylintrc +++ b/pylintrc @@ -19,7 +19,7 @@ persistent=yes # List of plugins (as comma separated values of python modules names) to load, # usually to register additional checkers. -load-plugins= +load-plugins=shopify_python [MESSAGES CONTROL] diff --git a/setup.py b/setup.py index 088d383..78dcca1 100755 --- a/setup.py +++ b/setup.py @@ -37,7 +37,8 @@ ], test_suite='tests', install_requires=[ - 'pylint==1.6.5' + 'pylint==1.6.5', + 'six>=1.10.0', ], extras_require={ 'dev': [ diff --git a/shopify_python/__init__.py b/shopify_python/__init__.py index 29d0584..384dcab 100644 --- a/shopify_python/__init__.py +++ b/shopify_python/__init__.py @@ -2,4 +2,12 @@ # Use of this source code is governed by a MIT-style license that can be found in the LICENSE file. from __future__ import unicode_literals -__version__ = '0.0.0' +from pylint import lint +from shopify_python import google_styleguide + + +__version__ = '0.1.0' + + +def register(linter): # type: (lint.PyLinter) -> None + google_styleguide.register_checkers(linter) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py new file mode 100644 index 0000000..a2d8efa --- /dev/null +++ b/shopify_python/google_styleguide.py @@ -0,0 +1,132 @@ +import sys + +import astroid # pylint: disable=unused-import +import six + +from pylint import checkers +from pylint import interfaces +from pylint import lint # pylint: disable=unused-import + + +if sys.version_info >= (3, 4): + import importlib.util # pylint: disable=no-name-in-module,import-error +else: + import importlib + + +def register_checkers(linter): # type: (lint.PyLinter) -> None + """Register checkers.""" + linter.register_checker(GoogleStyleGuideChecker(linter)) + + +class GoogleStyleGuideChecker(checkers.BaseChecker): + """ + Pylint checker for the Google Python Style Guide. + + See https://google.github.io/styleguide/pyguide.html + + Checks that can't be implemented include: + - When capturing an exception, use as rather than a comma + """ + __implements__ = (interfaces.IAstroidChecker,) + + name = 'google-styleguide-checker' + + msgs = { + 'C2601': ('Imported an object that is not a package or module', + 'import-modules-only', + 'Use imports for packages and modules only'), + 'C2602': ('Imported using a partial path', + 'import-full-path', + 'Import each module using the full pathname location of the module.'), + 'C2603': ('Variable declared at the module level (i.e. global)', + 'global-variable', + 'Avoid global variables in favor of class variables'), + 'C2604': ('Raised two-argument exception', + 'two-arg-exception', + "Use either raise Exception('message') or raise Exception"), + 'C2605': ('Raised deprecated string-exception', + 'string-exception', + "Use either raise Exception('message') or raise Exception"), + 'C2606': ('Caught StandardError', + 'catch-standard-error', + "Don't catch StandardError"), + } + + def visit_assign(self, node): # type: (astroid.Assign) -> None + self.__avoid_global_variables(node) + + def visit_excepthandler(self, node): # type: (astroid.ExceptHandler) -> None + self.__dont_catch_standard_error(node) + + def visit_importfrom(self, node): # type: (astroid.ImportFrom) -> None + self.__import_modules_only(node) + self.__import_full_path_only(node) + + def visit_raise(self, node): # type: (astroid.Raise) -> None + self.__dont_use_archaic_raise_syntax(node) + + def __import_modules_only(self, node): # type: (astroid.ImportFrom) -> None + """Use imports for packages and modules only.""" + + if hasattr(self.linter, 'config') and 'import-modules-only' in self.linter.config.disable: + return # Skip if disable to avoid side-effects from importing modules + + def can_import(module_name): + if sys.version_info >= (3, 4): + try: + return bool(importlib.util.find_spec(module_name)) + except AttributeError: + return False # Occurs when a module doesn't exist + except ImportError: + return False # Occurs when a module encounters an error during import + else: + try: + importlib.import_module(module_name) + return True + except ImportError: + return False # Occurs when a module doesn't exist or on error during import + + if not node.level and node.modname != '__future__': + for name in node.names: + name, _ = name + parent_module = node.modname + child_module = '.'.join((node.modname, name)) # Rearrange "from x import y" as "import x.y" + if can_import(parent_module) and not can_import(child_module): + self.add_message('import-modules-only', node=node) + + def __import_full_path_only(self, node): # type: (astroid.ImportFrom) -> None + """Import each module using the full pathname location of the module.""" + if node.level: + self.add_message('import-full-path', node=node) + + def __avoid_global_variables(self, node): # type: (astroid.Assign) -> None + """Avoid global variables.""" + # Is this an assignment happening within a module? If so report on each assignment name + # whether its in a tuple or not + if isinstance(node.parent, astroid.Module): + for target in node.targets: + if hasattr(target, 'elts'): + for elt in target.elts: + if elt.name != '__version__': + self.add_message('global-variable', node=elt) + elif hasattr(target, 'name'): + if target.name != '__version__': + self.add_message('global-variable', node=target) + + def __dont_use_archaic_raise_syntax(self, node): # type: (astroid.Raise) -> None + """Don't use the two-argument form of raise or the string raise""" + children = list(node.get_children()) + if len(children) > 1: + self.add_message('two-arg-exception', node=node) + elif len(children) == 1 and isinstance(children[0], six.string_types): + self.add_message('string-exception', node=node) + + def __dont_catch_standard_error(self, node): # type: (astroid.ExceptHandler) -> None + """ + Never use catch-all 'except:' statements, or catch Exception or StandardError. + + Pylint already handles bare-except and broad-except (for Exception). + """ + if hasattr(node.type, 'name') and node.type.name == 'StandardError': + self.add_message('catch-standard-error', node=node) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py new file mode 100644 index 0000000..e48c9eb --- /dev/null +++ b/tests/shopify_python/test_google_styleguide.py @@ -0,0 +1,91 @@ +import sys + +import astroid.test_utils +import pylint.testutils +import pytest + +from shopify_python import google_styleguide + + +class TestGoogleStyleGuideChecker(pylint.testutils.CheckerTestCase): + + CHECKER_CLASS = google_styleguide.GoogleStyleGuideChecker + + def test_importing_function_fails(self): + root = astroid.builder.parse(""" + from os.path import join + from io import FileIO + from os import environ + """) + messages = [] + for node in root.body: + messages.append(pylint.testutils.Message('import-modules-only', node=node)) + with self.assertAddsMessages(*messages): + self.walk(root) + + def test_importing_modules_passes(self): + root = astroid.builder.parse(""" + from __future__ import unicode_literals + from xml import dom + from xml import sax + from nonexistent_package import nonexistent_module + """) + with self.assertNoMessages(): + self.walk(root) + + def test_importing_relatively_fails(self): + root = astroid.builder.parse(""" + from . import string + from .. import string + """) + messages = [] + for node in root.body: + messages.append(pylint.testutils.Message('import-full-path', node=node)) + with self.assertAddsMessages(*messages): + self.walk(root) + + def test_global_variables_fail(self): + root = astroid.builder.parse(""" + module_var, other_module_var = 10 + __version__ = '0.0.0' + class MyClass(object): + class_var = 10 + """) + with self.assertAddsMessages( + pylint.testutils.Message('global-variable', node=root.body[0].targets[0].elts[0]), + pylint.testutils.Message('global-variable', node=root.body[0].targets[0].elts[1]), + ): + self.walk(root) + + @pytest.mark.skipif(sys.version_info >= (3, 0), reason="Tests code that is Python 3 incompatible") + def test_using_archaic_raise_fails(self): + root = astroid.builder.parse(""" + raise MyException, 'Error message' + raise 'Error message' + """) + node = root.body[0] + message = pylint.testutils.Message('two-arg-exception', node=node) + with self.assertAddsMessages(message): + self.walk(root) + + def test_catch_standard_error_fails(self): + root = astroid.builder.parse(""" + try: + pass + except StandardError: + pass + """) + node = root.body[0].handlers[0] + message = pylint.testutils.Message('catch-standard-error', node=node) + with self.assertAddsMessages(message): + self.walk(root) + + def test_catch_blank_passes(self): + root = astroid.builder.parse(""" + try: + pass + except: + pass + """) + with self.assertAddsMessages(): + self.walk(root)