From 0accf400d010905607d12abe9665bb81653edbcd Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 13 Mar 2017 11:33:07 -0400 Subject: [PATCH 1/4] Check that we disable pylint messages by name and not code --- shopify_python/__init__.py | 2 + shopify_python/shopify_styleguide.py | 43 +++++++++++++++++++ .../shopify_python/test_shopify_styleguide.py | 34 +++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 shopify_python/shopify_styleguide.py create mode 100644 tests/shopify_python/test_shopify_styleguide.py diff --git a/shopify_python/__init__.py b/shopify_python/__init__.py index e8c9224..d20b27e 100644 --- a/shopify_python/__init__.py +++ b/shopify_python/__init__.py @@ -4,6 +4,7 @@ from pylint import lint from shopify_python import google_styleguide +from shopify_python import shopify_styleguide __version__ = '0.1.2' @@ -11,3 +12,4 @@ def register(linter): # type: (lint.PyLinter) -> None google_styleguide.register_checkers(linter) + shopify_styleguide.register_checkers(linter) diff --git a/shopify_python/shopify_styleguide.py b/shopify_python/shopify_styleguide.py new file mode 100644 index 0000000..353343f --- /dev/null +++ b/shopify_python/shopify_styleguide.py @@ -0,0 +1,43 @@ +import re +import tokenize +import typing # pylint: disable=unused-import + +from pylint import checkers +from pylint import interfaces +from pylint import lint # pylint: disable=unused-import + + +def register_checkers(linter): # type: (lint.PyLinter) -> None + """Register checkers.""" + linter.register_checker(ShopifyStyleGuideChecker(linter)) + + +class ShopifyStyleGuideChecker(checkers.BaseTokenChecker): + """ + Pylint checker for Shopify-specific Code Style. + """ + __implements__ = (interfaces.ITokenChecker,) + + name = 'shopify-styleguide-checker' + + msgs = { + 'C2801': ('%(code)s disabled as a message code', + 'disable-name-only', + "Disable pylint rules via message name (e.g. unused-import) and not message code (e.g. W0611) to " + "help code reviewers understand why a linter rule was disabled for a line of code."), + } + + RE_PYLINT_DISABLE = re.compile(r'^#[ \t]*pylint:[ \t]*(disable|enable)[ \t]*=(?P[a-zA-Z0-9\-_, \t]+)$') + RE_PYLINT_MESSAGE_CODE = re.compile(r'^[A-Z]{1,2}[0-9]{4}$') + + def process_tokens(self, tokens): + # type: (typing.Sequence[typing.Tuple]) -> None + for _type, string, start, _, _ in tokens: + start_row, _ = start + if _type == tokenize.COMMENT: + matches = self.RE_PYLINT_DISABLE.match(string) + if matches: + for msg in matches.group('messages').split(','): + msg = msg.strip() + if self.RE_PYLINT_MESSAGE_CODE.match(msg): + self.add_message('disable-name-only', line=start_row, args={'code': msg}) diff --git a/tests/shopify_python/test_shopify_styleguide.py b/tests/shopify_python/test_shopify_styleguide.py new file mode 100644 index 0000000..b35a4fd --- /dev/null +++ b/tests/shopify_python/test_shopify_styleguide.py @@ -0,0 +1,34 @@ +import pylint.testutils + +from shopify_python import shopify_styleguide + + +class TestShopifyStyleGuideChecker(pylint.testutils.CheckerTestCase): + + CHECKER_CLASS = shopify_styleguide.ShopifyStyleGuideChecker + + def test_disable_name_only(self): + tokens = pylint.testutils.tokenize_str(""" + import os # pylint: disable=unused-import + import os # pylint: disable=unused-import,W0611 + import os # pylint: disable=W0611,C0302,C0303 + import os # pylint: disable=W0611 + import os #pylint:disable=W0611 + import os # pylint:\t\t disable \t\t = \t\t W0611 + def fnc(): + # pylint: disable=C0112 + pass + """.strip()) + + with self.assertAddsMessages(*[ + pylint.testutils.Message('disable-name-only', line=line, args={'code': code}) + for line, code in [(2, 'W0611'), + (3, 'W0611'), + (3, 'C0302'), + (3, 'C0303'), + (4, 'W0611'), + (5, 'W0611'), + (6, 'W0611'), + (8, 'C0112'), ] + ]): + self.checker.process_tokens(tokens) From 020a372ce556fd6723d9429b29fda56c21171470 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 13 Mar 2017 13:16:56 -0400 Subject: [PATCH 2/4] Change numbering scheme --- shopify_python/shopify_styleguide.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shopify_python/shopify_styleguide.py b/shopify_python/shopify_styleguide.py index 353343f..7b6ac01 100644 --- a/shopify_python/shopify_styleguide.py +++ b/shopify_python/shopify_styleguide.py @@ -21,7 +21,7 @@ class ShopifyStyleGuideChecker(checkers.BaseTokenChecker): name = 'shopify-styleguide-checker' msgs = { - 'C2801': ('%(code)s disabled as a message code', + 'C6101': ('%(code)s disabled as a message code', 'disable-name-only', "Disable pylint rules via message name (e.g. unused-import) and not message code (e.g. W0611) to " "help code reviewers understand why a linter rule was disabled for a line of code."), From 63335b4d2b144cc8cf4ef95a15534cfc37d2c3f2 Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 13 Mar 2017 13:51:54 -0400 Subject: [PATCH 3/4] Suggest message name --- shopify_python/shopify_styleguide.py | 16 ++++++++++++++-- tests/shopify_python/test_shopify_styleguide.py | 2 +- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/shopify_python/shopify_styleguide.py b/shopify_python/shopify_styleguide.py index 7b6ac01..70bd29c 100644 --- a/shopify_python/shopify_styleguide.py +++ b/shopify_python/shopify_styleguide.py @@ -2,6 +2,8 @@ import tokenize import typing # pylint: disable=unused-import +import pylint.utils + from pylint import checkers from pylint import interfaces from pylint import lint # pylint: disable=unused-import @@ -21,7 +23,7 @@ class ShopifyStyleGuideChecker(checkers.BaseTokenChecker): name = 'shopify-styleguide-checker' msgs = { - 'C6101': ('%(code)s disabled as a message code', + 'C6101': ("%(code)s disabled as a message code, use '%(name)s' instead", 'disable-name-only', "Disable pylint rules via message name (e.g. unused-import) and not message code (e.g. W0611) to " "help code reviewers understand why a linter rule was disabled for a line of code."), @@ -35,9 +37,19 @@ def process_tokens(self, tokens): for _type, string, start, _, _ in tokens: start_row, _ = start if _type == tokenize.COMMENT: + + def get_name(code): + if hasattr(self.linter, 'msgs_store'): + try: + return self.linter.msgs_store.get_msg_display_string(code) + except pylint.utils.UnknownMessage: + pass + return 'unknown' + matches = self.RE_PYLINT_DISABLE.match(string) if matches: for msg in matches.group('messages').split(','): msg = msg.strip() if self.RE_PYLINT_MESSAGE_CODE.match(msg): - self.add_message('disable-name-only', line=start_row, args={'code': msg}) + self.add_message('disable-name-only', line=start_row, + args={'code': msg, 'name': get_name(msg)}) diff --git a/tests/shopify_python/test_shopify_styleguide.py b/tests/shopify_python/test_shopify_styleguide.py index b35a4fd..28bf582 100644 --- a/tests/shopify_python/test_shopify_styleguide.py +++ b/tests/shopify_python/test_shopify_styleguide.py @@ -21,7 +21,7 @@ def fnc(): """.strip()) with self.assertAddsMessages(*[ - pylint.testutils.Message('disable-name-only', line=line, args={'code': code}) + pylint.testutils.Message('disable-name-only', line=line, args={'code': code, 'name': 'unknown'}) for line, code in [(2, 'W0611'), (3, 'W0611'), (3, 'C0302'), From 78f5236adae4cf9af46771d61eb1f5babecc925c Mon Sep 17 00:00:00 2001 From: Chris Fournier Date: Mon, 13 Mar 2017 17:11:27 -0400 Subject: [PATCH 4/4] Mock msgs_store --- setup.py | 1 + shopify_python/shopify_styleguide.py | 10 ++--- .../shopify_python/test_shopify_styleguide.py | 39 ++++++++++++++----- 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/setup.py b/setup.py index a5d5ab6..cc01ca1 100755 --- a/setup.py +++ b/setup.py @@ -50,6 +50,7 @@ def get_version(): 'pytest', 'pytest-randomly', 'mypy; python_version >= "3.3"', + 'mock; python_version < "3.3"', ] } ) diff --git a/shopify_python/shopify_styleguide.py b/shopify_python/shopify_styleguide.py index 70bd29c..e5e3aaf 100644 --- a/shopify_python/shopify_styleguide.py +++ b/shopify_python/shopify_styleguide.py @@ -39,12 +39,10 @@ def process_tokens(self, tokens): if _type == tokenize.COMMENT: def get_name(code): - if hasattr(self.linter, 'msgs_store'): - try: - return self.linter.msgs_store.get_msg_display_string(code) - except pylint.utils.UnknownMessage: - pass - return 'unknown' + try: + return self.linter.msgs_store.get_msg_display_string(code) + except pylint.utils.UnknownMessage: + return 'unknown' matches = self.RE_PYLINT_DISABLE.match(string) if matches: diff --git a/tests/shopify_python/test_shopify_styleguide.py b/tests/shopify_python/test_shopify_styleguide.py index 28bf582..7907f1b 100644 --- a/tests/shopify_python/test_shopify_styleguide.py +++ b/tests/shopify_python/test_shopify_styleguide.py @@ -1,5 +1,10 @@ import pylint.testutils +try: + import unittest.mock as mock # Python 3.3+ +except ImportError: + import mock # Python < 3.3 + from shopify_python import shopify_styleguide @@ -8,6 +13,14 @@ class TestShopifyStyleGuideChecker(pylint.testutils.CheckerTestCase): CHECKER_CLASS = shopify_styleguide.ShopifyStyleGuideChecker def test_disable_name_only(self): + + # Patch 'msgs_store' + mock_msgs_store = mock.Mock() + mock_msgs_store.get_msg_display_string = mock.Mock() + mock_msgs_store.get_msg_display_string.return_value = 'mocked' + setattr(self.linter, 'msgs_store', mock_msgs_store) + + # Create tokens tokens = pylint.testutils.tokenize_str(""" import os # pylint: disable=unused-import import os # pylint: disable=unused-import,W0611 @@ -20,15 +33,23 @@ def fnc(): pass """.strip()) + # Expected + expected_line_msgcodes = [ + (2, 'W0611'), + (3, 'W0611'), + (3, 'C0302'), + (3, 'C0303'), + (4, 'W0611'), + (5, 'W0611'), + (6, 'W0611'), + (8, 'C0112'), + ] + + # Test with self.assertAddsMessages(*[ - pylint.testutils.Message('disable-name-only', line=line, args={'code': code, 'name': 'unknown'}) - for line, code in [(2, 'W0611'), - (3, 'W0611'), - (3, 'C0302'), - (3, 'C0303'), - (4, 'W0611'), - (5, 'W0611'), - (6, 'W0611'), - (8, 'C0112'), ] + pylint.testutils.Message('disable-name-only', line=line, args={'code': code, 'name': 'mocked'}) + for line, code in expected_line_msgcodes ]): self.checker.process_tokens(tokens) + mock_msgs_store.get_msg_display_string.assert_has_calls( + [mock.call(code) for _, code in expected_line_msgcodes])