From 4963d398b8f2472e377b391915e2fc622f7f2bd5 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Wed, 1 Mar 2017 09:31:19 -0500 Subject: [PATCH 01/17] Add Google Style Guide import-modules-only and import-full-path rules --- .travis.yml | 1 - Makefile | 2 +- pylintrc | 2 +- shopify_python/__init__.py | 8 +++ shopify_python/google_styleguide.py | 55 +++++++++++++++++++ .../shopify_python/test_google_styleguide.py | 40 ++++++++++++++ 6 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 shopify_python/google_styleguide.py create mode 100644 tests/shopify_python/test_google_styleguide.py 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/shopify_python/__init__.py b/shopify_python/__init__.py index 29d0584..c28fdeb 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 +from pylint import lint +from shopify_python import google_styleguide + + __version__ = '0.0.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..a5b0b78 --- /dev/null +++ b/shopify_python/google_styleguide.py @@ -0,0 +1,55 @@ +import importlib + +import astroid # pylint: disable=unused-import + +from pylint import checkers, interfaces + + +def register_checkers(linter): # type: (astroid.ImportFrom) -> None + """Register checkers.""" + linter.register_checker(GoogleStyleGuideChecker(linter)) + + +class GoogleStyleGuideChecker(checkers.BaseChecker): + __implements__ = (interfaces.IAstroidChecker,) + + name = 'google-styleguide-checker' + + msgs = { + 'C9901': ('Imported an object that is not a package or module', + 'import-modules-only', + 'Use imports for packages and modules only.'), + 'C9902': ('Imported using a partial path', + 'import-full-path', + 'Import each module using the full pathname location of the module.') + } + + def visit_importfrom(self, node): # type: (astroid.ImportFrom) -> None + self._import_modules_only(node) + self._import_full_path_only(node) + + def _import_modules_only(self, node): # type: (astroid.ImportFrom) -> None + """ + Use imports for packages and modules only. + + See https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports + """ + if not node.level and node.modname != '__future__': + for name in node.names: + # Try to rearrange "from x import y" as "import x.y" and import it as a module + name, _ = name + module = '.'.join((node.modname, name)) + try: + importlib.import_module(module) + except ImportError as import_error: + if str(import_error).startswith('No module named'): + 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. + + See https://google.github.io/styleguide/pyguide.html?showone=Packages#Packages + """ + if node.level: + self.add_message('import-full-path', 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..2c3a59b --- /dev/null +++ b/tests/shopify_python/test_google_styleguide.py @@ -0,0 +1,40 @@ +import astroid.test_utils +import pylint.testutils + +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 + """) + node = root.__dict__['body'][0] + message = pylint.testutils.Message('import-modules-only', + node=node) + with self.assertAddsMessages(message): + self.walk(root) + + def test_importing_modules_passes(self): + root = astroid.builder.parse(""" + from __future__ import unicode_literals + import sys + from xml import dom + from xml import sax + """) + 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.__dict__['body']: + messages.append(pylint.testutils.Message('import-full-path', node=node)) + with self.assertAddsMessages(*messages): + self.walk(root) From 011c5ea613f55ee83cc582d5beb277c951e2ce3a Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Thu, 2 Mar 2017 19:42:52 -0500 Subject: [PATCH 02/17] Support importing nonesitent module --- shopify_python/google_styleguide.py | 17 +++++++++++------ tests/shopify_python/test_google_styleguide.py | 2 +- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index a5b0b78..2bfe532 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -34,16 +34,21 @@ def _import_modules_only(self, node): # type: (astroid.ImportFrom) -> None See https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports """ + def can_import(module): + try: + importlib.import_module(module) + return True + except ImportError: + return False + if not node.level and node.modname != '__future__': for name in node.names: # Try to rearrange "from x import y" as "import x.y" and import it as a module name, _ = name - module = '.'.join((node.modname, name)) - try: - importlib.import_module(module) - except ImportError as import_error: - if str(import_error).startswith('No module named'): - self.add_message('import-modules-only', node=node) + parent_module = node.modname + child_module = '.'.join((node.modname, name)) + 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 """ diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 2c3a59b..d38b339 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -21,9 +21,9 @@ def test_importing_function_fails(self): def test_importing_modules_passes(self): root = astroid.builder.parse(""" from __future__ import unicode_literals - import sys from xml import dom from xml import sax + from nonexistent_package import nonexistent_module """) with self.assertNoMessages(): self.walk(root) From 6ff8a4661c4edfb7317207e8effc79aa1402a8cc Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Thu, 2 Mar 2017 19:46:23 -0500 Subject: [PATCH 03/17] Add module doc string --- shopify_python/google_styleguide.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 2bfe532..6d2c9d0 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -11,6 +11,11 @@ def register_checkers(linter): # type: (astroid.ImportFrom) -> None class GoogleStyleGuideChecker(checkers.BaseChecker): + """ + Pylint checker for the Google Python Style Guide. + + See https://google.github.io/styleguide/pyguide.html + """ __implements__ = (interfaces.IAstroidChecker,) name = 'google-styleguide-checker' From 40ce17133d0d5df260cf4c0d9a4270105b3602a9 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 11:12:21 -0500 Subject: [PATCH 04/17] Add a few more rules and make functions private --- setup.py | 3 +- shopify_python/google_styleguide.py | 99 ++++++++++++++++--- .../shopify_python/test_google_styleguide.py | 40 +++++++- 3 files changed, 123 insertions(+), 19 deletions(-) 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/google_styleguide.py b/shopify_python/google_styleguide.py index 6d2c9d0..58584b8 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -1,6 +1,7 @@ import importlib import astroid # pylint: disable=unused-import +import six from pylint import checkers, interfaces @@ -23,22 +24,39 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): msgs = { 'C9901': ('Imported an object that is not a package or module', 'import-modules-only', - 'Use imports for packages and modules only.'), + 'Use imports for packages and modules only'), 'C9902': ('Imported using a partial path', 'import-full-path', - 'Import each module using the full pathname location of the module.') + 'Import each module using the full pathname location of the module.'), + 'C9903': ('Variable declared at the module level (i.e. global)', + 'global-variable', + 'Avoid global variables in favor of class variables'), + 'C9904': ('Raised two-argument exception', + 'two-arg-exception', + "Use either raise Exception('message') or raise Exception"), + 'C9905': ('Raised deprecated string-exception', + 'string-exception', + "Use either raise Exception('message') or raise Exception"), + 'C9906': ('Caught StandardError', + 'catch-standard-error', + "Don't catch broad exceptions"), } + 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) + self.__import_modules_only(node) + self.__import_full_path_only(node) - def _import_modules_only(self, node): # type: (astroid.ImportFrom) -> None - """ - Use imports for packages and modules only. + def visit_raise(self, node): # type: (astroid.Raise) -> None + self.__dont_use_archaic_raise_syntax(node) - See https://google.github.io/styleguide/pyguide.html?showone=Imports#Imports - """ + def __import_modules_only(self, node): # type: (astroid.ImportFrom) -> None + """Use imports for packages and modules only.""" def can_import(module): try: importlib.import_module(module) @@ -48,18 +66,67 @@ def can_import(module): if not node.level and node.modname != '__future__': for name in node.names: - # Try to rearrange "from x import y" as "import x.y" and import it as a module name, _ = name parent_module = node.modname - child_module = '.'.join((node.modname, name)) + 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 + 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.""" + if isinstance(node.parent, astroid.Module): + self.add_message('global-variable', node=node) + + 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 """ - Import each module using the full pathname location of the module. + Never use catch-all 'except:' statements, or catch Exception or StandardError. - See https://google.github.io/styleguide/pyguide.html?showone=Packages#Packages + Pylint already handles bare-except and broad-except (for Exception). """ - if node.level: - self.add_message('import-full-path', node=node) + if node.type.name == 'StandardError': + self.add_message('catch-standard-error', node=node) + + # TODO Exceptions are allowed but must be used carefully. + # - Never use catch-all except: statements, or catch Exception or StandardError, + # - Covered by bare-except, broad-except, but not StandardError + # - When capturing an exception, use as rather than a comma. For example: + # - Minimize the amount of code in a try/except block. + + # TODO List comprehensions are okay to use for simple cases. + # def visit_comprehension, check parent, it it contains multiple generators then that's bad + + # TODO Use default iterators and operators for types that support them, like lists, dictionaries, and files. + + # TODO Lambdas are okay for one-liners. + # TODO Conditional expressions are okay for one-liners. + + # TODO Do not use mutable objects as default values in the function or + # method definition. (is this covered by pylint already?) + + # TODO Use properties for accessing or setting data where you would + # normally have used simple, lightweight accessor or setter methods. + + # TODO Use the "implicit" false if at all possible. + + # TODO Use string methods instead of the string module where possible. (Covered by pylint) + + # TODO Avoid fancy features like metaclasses, access to bytecode, + # on-the-fly compilation, dynamic inheritance, object reparenting, import + # hacks, reflection, modification of system internals, etc. + + # TODO Shebang lines shoul dbe of the form #!/usr/bin/env python with an optional single digit 2 or 3 suffix + + # TODO If a class inherits from no other base classes, explicitly inherit from object. (already covered by pylint?) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index d38b339..45d9984 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -1,5 +1,8 @@ +import sys + import astroid.test_utils import pylint.testutils +import pytest from shopify_python import google_styleguide @@ -13,8 +16,7 @@ def test_importing_function_fails(self): from os.path import join """) node = root.__dict__['body'][0] - message = pylint.testutils.Message('import-modules-only', - node=node) + message = pylint.testutils.Message('import-modules-only', node=node) with self.assertAddsMessages(message): self.walk(root) @@ -38,3 +40,37 @@ def test_importing_relatively_fails(self): 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 = 10 + class MyClass(object): + class_var = 10 + """) + node = root.__dict__['body'][0] + message = pylint.testutils.Message('global-variable', node=node) + with self.assertAddsMessages(message): + 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.__dict__['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.__dict__['body'][0].__dict__['handlers'][0] + message = pylint.testutils.Message('catch-standard-error', node=node) + with self.assertAddsMessages(message): + self.walk(root) From dcf85c769cbd937f2003c2b588cdc3689e75cb1a Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 14:15:39 -0500 Subject: [PATCH 05/17] Add more rules and check for import rule disable --- shopify_python/google_styleguide.py | 77 +++++++++++-------- .../shopify_python/test_google_styleguide.py | 63 ++++++++++++++- 2 files changed, 103 insertions(+), 37 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 58584b8..d601500 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -6,7 +6,7 @@ from pylint import checkers, interfaces -def register_checkers(linter): # type: (astroid.ImportFrom) -> None +def register_checkers(linter): # type: (lint.PyLinter) -> None """Register checkers.""" linter.register_checker(GoogleStyleGuideChecker(linter)) @@ -16,6 +16,9 @@ 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,) @@ -40,14 +43,31 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): 'C9906': ('Caught StandardError', 'catch-standard-error', "Don't catch broad exceptions"), + 'C9907': ('Try body too long', + 'try-too-long', + "The larger the try body, the more likely that an unexpected exception will be raised"), + 'C9908': ('Except body too long', + 'except-too-long', + "The larger the except body, the more likely that an exception will be raised"), + 'C9909': ('Finally body too long', + 'finally-too-long', + "The larger the except body, the more likely that an exception will be raised"), } + max_try_exc_finally_body_size = 5 + 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_tryexcept(self, node): # type: (astroid.TryExcept) -> None + self.__minimize_code_in_try_except(node) + + def visit_tryfinally(self, node): # type: (astroid.TryFinally) -> None + self.__minimize_code_in_finally(node) + def visit_importfrom(self, node): # type: (astroid.ImportFrom) -> None self.__import_modules_only(node) self.__import_full_path_only(node) @@ -57,6 +77,9 @@ def visit_raise(self, node): # type: (astroid.Raise) -> None 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): try: importlib.import_module(module) @@ -80,7 +103,14 @@ def __import_full_path_only(self, node): # type: (astroid.ImportFrom) -> None def __avoid_global_variables(self, node): # type: (astroid.Assign) -> None """Avoid global variables.""" if isinstance(node.parent, astroid.Module): - self.add_message('global-variable', node=node) + 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""" @@ -99,34 +129,15 @@ def __dont_catch_standard_error(self, node): # type: (astroid.ExceptHandler) -> if node.type.name == 'StandardError': self.add_message('catch-standard-error', node=node) - # TODO Exceptions are allowed but must be used carefully. - # - Never use catch-all except: statements, or catch Exception or StandardError, - # - Covered by bare-except, broad-except, but not StandardError - # - When capturing an exception, use as rather than a comma. For example: - # - Minimize the amount of code in a try/except block. - - # TODO List comprehensions are okay to use for simple cases. - # def visit_comprehension, check parent, it it contains multiple generators then that's bad - - # TODO Use default iterators and operators for types that support them, like lists, dictionaries, and files. - - # TODO Lambdas are okay for one-liners. - # TODO Conditional expressions are okay for one-liners. - - # TODO Do not use mutable objects as default values in the function or - # method definition. (is this covered by pylint already?) - - # TODO Use properties for accessing or setting data where you would - # normally have used simple, lightweight accessor or setter methods. - - # TODO Use the "implicit" false if at all possible. - - # TODO Use string methods instead of the string module where possible. (Covered by pylint) - - # TODO Avoid fancy features like metaclasses, access to bytecode, - # on-the-fly compilation, dynamic inheritance, object reparenting, import - # hacks, reflection, modification of system internals, etc. - - # TODO Shebang lines shoul dbe of the form #!/usr/bin/env python with an optional single digit 2 or 3 suffix - - # TODO If a class inherits from no other base classes, explicitly inherit from object. (already covered by pylint?) + def __minimize_code_in_try_except(self, node): # type: (astroid.TryExcept) -> None + """Minimize the amount of code in a try/except block.""" + if len(node.body) > self.max_try_exc_finally_body_size: + self.add_message('try-too-long', node=node) + for handler in node.handlers: + if len(handler.body) > self.max_try_exc_finally_body_size: + self.add_message('except-too-long', node=handler) + + def __minimize_code_in_finally(self, node): # type: (astroid.TryFinally) -> None + """Minimize the amount of code in a finally block.""" + if len(node.finalbody) > self.max_try_exc_finally_body_size: + self.add_message('finally-too-long', node=node) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 45d9984..6aaedee 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -43,13 +43,15 @@ def test_importing_relatively_fails(self): def test_global_variables_fail(self): root = astroid.builder.parse(""" - module_var = 10 + module_var, other_module_var = 10 + __version__ = '0.0.0' class MyClass(object): class_var = 10 """) - node = root.__dict__['body'][0] - message = pylint.testutils.Message('global-variable', node=node) - with self.assertAddsMessages(message): + 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") @@ -74,3 +76,56 @@ def test_catch_standard_error_fails(self): message = pylint.testutils.Message('catch-standard-error', node=node) with self.assertAddsMessages(message): self.walk(root) + + def test_try_exc_finally_size(self): + root = astroid.builder.parse(""" + try: + # Comments are OK. + # They are needed to document + # complicated exception + # scenarious and should + # not be penalized. + l = 1 + except AssertionError: + # Comments are OK. + # They are needed to document + # complicated exception + # scenarious and should + # not be penalized. + raise + finally: + # Comments are OK. + # They are needed to document + # complicated exception + # scenarious and should + # not be penalized. + l = 1 + + try: + l = 1 + l = 2 + l = 3 + l = 4 + l = 5 + l = 6 + except AssertionError: + l = 1 + l = 2 + l = 3 + l = 4 + l = 5 + l = 6 + finally: + l = 1 + l = 2 + l = 3 + l = 4 + l = 5 + l = 6 + """) + with self.assertAddsMessages( + pylint.testutils.Message('finally-too-long', node=root.body[1]), + pylint.testutils.Message('try-too-long', node=root.body[1].body[0]), + pylint.testutils.Message('except-too-long', node=root.body[1].body[0].handlers[0]), + ): + self.walk(root) From e7a78b04e32cb5caa857d43988a1a57818353f29 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 14:36:02 -0500 Subject: [PATCH 06/17] Change message id prefixes --- shopify_python/google_styleguide.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index d601500..5ca2dd4 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -25,31 +25,31 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): name = 'google-styleguide-checker' msgs = { - 'C9901': ('Imported an object that is not a package or module', + 'C2601': ('Imported an object that is not a package or module', 'import-modules-only', 'Use imports for packages and modules only'), - 'C9902': ('Imported using a partial path', + 'C2602': ('Imported using a partial path', 'import-full-path', 'Import each module using the full pathname location of the module.'), - 'C9903': ('Variable declared at the module level (i.e. global)', + 'C2603': ('Variable declared at the module level (i.e. global)', 'global-variable', 'Avoid global variables in favor of class variables'), - 'C9904': ('Raised two-argument exception', + 'C2604': ('Raised two-argument exception', 'two-arg-exception', "Use either raise Exception('message') or raise Exception"), - 'C9905': ('Raised deprecated string-exception', + 'C2605': ('Raised deprecated string-exception', 'string-exception', "Use either raise Exception('message') or raise Exception"), - 'C9906': ('Caught StandardError', + 'C2606': ('Caught StandardError', 'catch-standard-error', "Don't catch broad exceptions"), - 'C9907': ('Try body too long', + 'C2607': ('Try body too long', 'try-too-long', "The larger the try body, the more likely that an unexpected exception will be raised"), - 'C9908': ('Except body too long', + 'C2608': ('Except body too long', 'except-too-long', "The larger the except body, the more likely that an exception will be raised"), - 'C9909': ('Finally body too long', + 'C2609': ('Finally body too long', 'finally-too-long', "The larger the except body, the more likely that an exception will be raised"), } From cef8dbf1bbdc724a0ce0fadfa254aaf1f86907a3 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 14:38:05 -0500 Subject: [PATCH 07/17] Remove __dict__ usage --- tests/shopify_python/test_google_styleguide.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 6aaedee..d398015 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -15,7 +15,7 @@ def test_importing_function_fails(self): root = astroid.builder.parse(""" from os.path import join """) - node = root.__dict__['body'][0] + node = root.body[0] message = pylint.testutils.Message('import-modules-only', node=node) with self.assertAddsMessages(message): self.walk(root) @@ -36,7 +36,7 @@ def test_importing_relatively_fails(self): from .. import string """) messages = [] - for node in root.__dict__['body']: + for node in root.body: messages.append(pylint.testutils.Message('import-full-path', node=node)) with self.assertAddsMessages(*messages): self.walk(root) @@ -60,7 +60,7 @@ def test_using_archaic_raise_fails(self): raise MyException, 'Error message' raise 'Error message' """) - node = root.__dict__['body'][0] + node = root.body[0] message = pylint.testutils.Message('two-arg-exception', node=node) with self.assertAddsMessages(message): self.walk(root) @@ -72,7 +72,7 @@ def test_catch_standard_error_fails(self): except StandardError: pass """) - node = root.__dict__['body'][0].__dict__['handlers'][0] + node = root.body[0].handlers[0] message = pylint.testutils.Message('catch-standard-error', node=node) with self.assertAddsMessages(message): self.walk(root) From 0c2bff0967fda2837d796b95d2ea238b4786bf11 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 14:43:26 -0500 Subject: [PATCH 08/17] Add tests for importing a class and module --- tests/shopify_python/test_google_styleguide.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index d398015..8be1e2f 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -14,10 +14,13 @@ class TestGoogleStyleGuideChecker(pylint.testutils.CheckerTestCase): def test_importing_function_fails(self): root = astroid.builder.parse(""" from os.path import join + from io import FileIO + from os import environ """) - node = root.body[0] - message = pylint.testutils.Message('import-modules-only', node=node) - with self.assertAddsMessages(message): + 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): From 752a505f9400b980c00db949e20761b56fd3e2bc Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 15:45:55 -0500 Subject: [PATCH 09/17] For Python 3 use importlib.utils.find_spec --- shopify_python/google_styleguide.py | 31 +++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 5ca2dd4..c7ab2e3 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -1,9 +1,17 @@ -import importlib +import sys import astroid # pylint: disable=unused-import import six -from pylint import checkers, interfaces +from pylint import checkers +from pylint import interfaces +from pylint import lint # pylint: disable=unused-import + + +if sys.version_info >= (3, 0): + import importlib.util # pylint: disable=no-name-in-module,import-error +else: + import importlib def register_checkers(linter): # type: (lint.PyLinter) -> None @@ -77,15 +85,22 @@ def visit_raise(self, node): # type: (astroid.Raise) -> None 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): - try: - importlib.import_module(module) - return True - except ImportError: - return False + def can_import(module_name): + if sys.version_info >= (3, 0): + try: + return bool(importlib.util.find_spec(module_name)) + except AttributeError: + return False + else: + try: + importlib.import_module(module_name) + return True + except ImportError: + return False if not node.level and node.modname != '__future__': for name in node.names: From a021916d9d2aba18c6c766d8f1644ece4196879e Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 15:49:50 -0500 Subject: [PATCH 10/17] Scope find_spec usage to Python 3.4+ and test more Python 3.x versions --- .travis.yml | 3 +++ shopify_python/google_styleguide.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index f35ff8f..3a732c0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,6 +3,9 @@ language: python python: - "2.7" + - "3.0" + - "3.1" + - "3.2" - "3.3" - "3.4" - "3.5" diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index c7ab2e3..1dcea96 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -8,7 +8,7 @@ from pylint import lint # pylint: disable=unused-import -if sys.version_info >= (3, 0): +if sys.version_info >= (3, 4): import importlib.util # pylint: disable=no-name-in-module,import-error else: import importlib @@ -90,7 +90,7 @@ def __import_modules_only(self, node): # type: (astroid.ImportFrom) -> None return # Skip if disable to avoid side-effects from importing modules def can_import(module_name): - if sys.version_info >= (3, 0): + if sys.version_info >= (3, 4): try: return bool(importlib.util.find_spec(module_name)) except AttributeError: From f2969cd1e4db812833587d4328a69a12bca8ffb8 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 15:53:35 -0500 Subject: [PATCH 11/17] Nope, Python 3.0 to 3.2 are broken --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3a732c0..f35ff8f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,9 +3,6 @@ language: python python: - "2.7" - - "3.0" - - "3.1" - - "3.2" - "3.3" - "3.4" - "3.5" From 36e002fcd51f36241e149d5f436995a7b84d8b0b Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 16:40:23 -0500 Subject: [PATCH 12/17] Be more specific about which broad exception shouldn't be caught --- shopify_python/google_styleguide.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 1dcea96..c1b0fff 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -50,7 +50,7 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): "Use either raise Exception('message') or raise Exception"), 'C2606': ('Caught StandardError', 'catch-standard-error', - "Don't catch broad exceptions"), + "Don't catch StandardError"), 'C2607': ('Try body too long', 'try-too-long', "The larger the try body, the more likely that an unexpected exception will be raised"), From beb2121af40be24367a578b95f3cf0129650eb98 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Fri, 3 Mar 2017 16:40:39 -0500 Subject: [PATCH 13/17] Explain how the global var check works --- shopify_python/google_styleguide.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index c1b0fff..b3526a8 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -117,6 +117,8 @@ def __import_full_path_only(self, node): # type: (astroid.ImportFrom) -> None 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'): From d410ee919008e480900e6ef39edf8c4786e2503f Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 6 Mar 2017 11:07:40 -0500 Subject: [PATCH 14/17] Check for whether a catch has an exception --- shopify_python/google_styleguide.py | 2 +- tests/shopify_python/test_google_styleguide.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index b3526a8..8115295 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -143,7 +143,7 @@ def __dont_catch_standard_error(self, node): # type: (astroid.ExceptHandler) -> Pylint already handles bare-except and broad-except (for Exception). """ - if node.type.name == 'StandardError': + if hasattr(node.type, 'name') and node.type.name == 'StandardError': self.add_message('catch-standard-error', node=node) def __minimize_code_in_try_except(self, node): # type: (astroid.TryExcept) -> None diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index 8be1e2f..a1368ff 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -80,6 +80,16 @@ def test_catch_standard_error_fails(self): 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) + def test_try_exc_finally_size(self): root = astroid.builder.parse(""" try: From 5c9b22bef5d0459a1ec4eca37945dd1aa5024d41 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 6 Mar 2017 11:08:41 -0500 Subject: [PATCH 15/17] Bump version because we added features --- shopify_python/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shopify_python/__init__.py b/shopify_python/__init__.py index c28fdeb..384dcab 100644 --- a/shopify_python/__init__.py +++ b/shopify_python/__init__.py @@ -6,7 +6,7 @@ from shopify_python import google_styleguide -__version__ = '0.0.0' +__version__ = '0.1.0' def register(linter): # type: (lint.PyLinter) -> None From 541710e7f42b72b7517b551568da23abf70dc1bf Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 6 Mar 2017 11:22:51 -0500 Subject: [PATCH 16/17] Punt the try/except/finally check to a future PR --- shopify_python/google_styleguide.py | 30 ----------- .../shopify_python/test_google_styleguide.py | 53 ------------------- 2 files changed, 83 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 8115295..6c4d7b5 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -51,31 +51,14 @@ class GoogleStyleGuideChecker(checkers.BaseChecker): 'C2606': ('Caught StandardError', 'catch-standard-error', "Don't catch StandardError"), - 'C2607': ('Try body too long', - 'try-too-long', - "The larger the try body, the more likely that an unexpected exception will be raised"), - 'C2608': ('Except body too long', - 'except-too-long', - "The larger the except body, the more likely that an exception will be raised"), - 'C2609': ('Finally body too long', - 'finally-too-long', - "The larger the except body, the more likely that an exception will be raised"), } - max_try_exc_finally_body_size = 5 - 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_tryexcept(self, node): # type: (astroid.TryExcept) -> None - self.__minimize_code_in_try_except(node) - - def visit_tryfinally(self, node): # type: (astroid.TryFinally) -> None - self.__minimize_code_in_finally(node) - def visit_importfrom(self, node): # type: (astroid.ImportFrom) -> None self.__import_modules_only(node) self.__import_full_path_only(node) @@ -145,16 +128,3 @@ def __dont_catch_standard_error(self, node): # type: (astroid.ExceptHandler) -> """ if hasattr(node.type, 'name') and node.type.name == 'StandardError': self.add_message('catch-standard-error', node=node) - - def __minimize_code_in_try_except(self, node): # type: (astroid.TryExcept) -> None - """Minimize the amount of code in a try/except block.""" - if len(node.body) > self.max_try_exc_finally_body_size: - self.add_message('try-too-long', node=node) - for handler in node.handlers: - if len(handler.body) > self.max_try_exc_finally_body_size: - self.add_message('except-too-long', node=handler) - - def __minimize_code_in_finally(self, node): # type: (astroid.TryFinally) -> None - """Minimize the amount of code in a finally block.""" - if len(node.finalbody) > self.max_try_exc_finally_body_size: - self.add_message('finally-too-long', node=node) diff --git a/tests/shopify_python/test_google_styleguide.py b/tests/shopify_python/test_google_styleguide.py index a1368ff..e48c9eb 100644 --- a/tests/shopify_python/test_google_styleguide.py +++ b/tests/shopify_python/test_google_styleguide.py @@ -89,56 +89,3 @@ def test_catch_blank_passes(self): """) with self.assertAddsMessages(): self.walk(root) - - def test_try_exc_finally_size(self): - root = astroid.builder.parse(""" - try: - # Comments are OK. - # They are needed to document - # complicated exception - # scenarious and should - # not be penalized. - l = 1 - except AssertionError: - # Comments are OK. - # They are needed to document - # complicated exception - # scenarious and should - # not be penalized. - raise - finally: - # Comments are OK. - # They are needed to document - # complicated exception - # scenarious and should - # not be penalized. - l = 1 - - try: - l = 1 - l = 2 - l = 3 - l = 4 - l = 5 - l = 6 - except AssertionError: - l = 1 - l = 2 - l = 3 - l = 4 - l = 5 - l = 6 - finally: - l = 1 - l = 2 - l = 3 - l = 4 - l = 5 - l = 6 - """) - with self.assertAddsMessages( - pylint.testutils.Message('finally-too-long', node=root.body[1]), - pylint.testutils.Message('try-too-long', node=root.body[1].body[0]), - pylint.testutils.Message('except-too-long', node=root.body[1].body[0].handlers[0]), - ): - self.walk(root) From a81732d9da3c2204db4f387e8b2e58a93f134291 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 6 Mar 2017 12:14:19 -0500 Subject: [PATCH 17/17] Handle import error --- shopify_python/google_styleguide.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/shopify_python/google_styleguide.py b/shopify_python/google_styleguide.py index 6c4d7b5..a2d8efa 100644 --- a/shopify_python/google_styleguide.py +++ b/shopify_python/google_styleguide.py @@ -77,13 +77,15 @@ def can_import(module_name): try: return bool(importlib.util.find_spec(module_name)) except AttributeError: - return False + 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 + 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: