From 46a7fc8143df9208490bdef54a156f8a64d307a4 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Sat, 27 Oct 2018 13:53:54 +0300 Subject: [PATCH] Restricts to use protected attributes, closes #272 --- CHANGELOG.md | 1 + tests/fixtures/noqa.py | 5 +- tests/test_checker/test_noqa.py | 3 + tests/test_violations/test_implementation.py | 2 +- .../test_protected_attributes.py | 110 ++++++++++++++++++ .../test_formatted_string.py | 8 +- .../test_magic_numbers.py | 6 +- .../test_classes/test_yield_inside_init.py | 14 +++ .../test_jones/test_line_complexity.py | 12 +- .../test_imports/test_protected_import.py | 22 +++- .../test_ast/test_keywords/test_loops.py | 100 +++++++++++++++- wemake_python_styleguide/constants.py | 5 +- .../logics/variables/access.py | 11 +- .../logics/variables/name_nodes.py | 2 +- .../logics/variables/naming.py | 60 ++-------- .../violations/best_practices.py | 106 ++++++++++++++++- wemake_python_styleguide/violations/naming.py | 45 +------ .../visitors/ast/attributes.py | 39 +++++++ .../visitors/ast/{numbers.py => builtins.py} | 25 +++- .../visitors/ast/imports.py | 6 +- .../visitors/ast/keywords.py | 45 +++++-- .../visitors/ast/strings.py | 23 ---- .../visitors/presets/general.py | 10 +- 23 files changed, 488 insertions(+), 172 deletions(-) create mode 100644 tests/test_visitors/test_ast/test_attributes/test_protected_attributes.py rename tests/test_visitors/test_ast/{test_strings => test_builtins}/test_formatted_string.py (84%) rename tests/test_visitors/test_ast/{test_numbers => test_builtins}/test_magic_numbers.py (95%) create mode 100644 wemake_python_styleguide/visitors/ast/attributes.py rename wemake_python_styleguide/visitors/ast/{numbers.py => builtins.py} (73%) delete mode 100644 wemake_python_styleguide/visitors/ast/strings.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 51d78ed88..cad457014 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,7 @@ to the project during `#hactoberfest`. List of awesome people: - Forbids variable names with more than one consecutive underscore - Restricts the maximum number of base classes aka mixins - Forbids importing protected names +- Forbids using protected methods and attributes - Forbids `yield` inside `__init__` method ### Bugfixes diff --git a/tests/fixtures/noqa.py b/tests/fixtures/noqa.py index ccf4e0ca1..219669a95 100644 --- a/tests/fixtures/noqa.py +++ b/tests/fixtures/noqa.py @@ -8,6 +8,7 @@ from .version import get_version # noqa: Z300 import sys as sys # noqa: Z113 +from some import _protected # noqa: Z440 full_name = u'Nikita Sobolev' # noqa: Z302 phone_number = 555_123_999 # noqa: Z303 @@ -29,9 +30,11 @@ def nested(): ... # noqa: Z430 value = 1 # noqa: Z110 x = 2 # noqa: Z111 __private = 3 # noqa: Z112 +consecutive__underscores = 4 # noqa: Z116 __author__ = 'Nikita Sobolev' # noqa: Z410 nodes = [node for node in 'abc' if node != 'a' if node != 'b'] # noqa: Z307 +some._execute() # noqa: Z441 class BadClass: # noqa: Z306 @@ -63,7 +66,7 @@ class Nested: # noqa: Z306,Z431 assert hex_number == hex_number # noqa: Z312 for symbol in 'abc': # noqa: Z436 - break + ... else: ... diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index 53763cb8d..cdc4ec4ca 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -25,6 +25,7 @@ def test_noqa_fixture_disabled(absolute_path): 'Z111': 1, 'Z112': 1, 'Z113': 1, + 'Z116': 1, 'Z220': 1, 'Z224': 1, @@ -54,6 +55,8 @@ def test_noqa_fixture_disabled(absolute_path): 'Z435': 1, 'Z436': 1, 'Z437': 1, + 'Z440': 1, + 'Z441': 1, } process = subprocess.Popen( diff --git a/tests/test_violations/test_implementation.py b/tests/test_violations/test_implementation.py index a76271da2..95189095e 100644 --- a/tests/test_violations/test_implementation.py +++ b/tests/test_violations/test_implementation.py @@ -15,4 +15,4 @@ def test_visitor_returns_location(): def test_checker_default_location(): """Ensures that `BaseViolation` returns correct location.""" - assert BaseViolation(None)._location() == (0, 0) + assert BaseViolation(None)._location() == (0, 0) # noqa: Z441 diff --git a/tests/test_visitors/test_ast/test_attributes/test_protected_attributes.py b/tests/test_visitors/test_ast/test_attributes/test_protected_attributes.py new file mode 100644 index 000000000..0cf503412 --- /dev/null +++ b/tests/test_visitors/test_ast/test_attributes/test_protected_attributes.py @@ -0,0 +1,110 @@ +# -*- coding: utf-8 -*- + +import pytest + +from wemake_python_styleguide.visitors.ast.attributes import ( + ProtectedAttributeViolation, + WrongAttributeVisitor, +) + +# Incorrect: + +protected_attribute_assigned = 'some._protected = 1' +protected_attribute_accessed = 'print(some._protected)' +protected_method_called = 'some._method()' +protected_method_called_params = 'some._method(12, 33)' + +protected_container_attribute = """ +class Test(object): + def __init__(self): + self.container._print = 1 +""" + +protected_container_method = """ +class Test(object): + def __init__(self): + self.container._print() +""" + +# Correct: + +protected_name_definition = '_protected = 1' +protected_name_attr_definition = '_protected.some = 1' + +protected_self_attribute = """ +class Test(object): + def __init__(self): + self._print = 1 +""" + +protected_self_method = """ +class Test(object): + def __init__(self): + self._print() +""" + +protected_cls_attribute = """ +class Test(object): + @classmethod + def method(cls): + cls._print = 'some' +""" + +protected_cls_method = """ +class Test(object): + @classmethod + def method(cls): + cls._print() +""" + +protected_attribute_definition = """ +class Test(object): + _protected = 1 +""" + + +@pytest.mark.parametrize('code', [ + protected_attribute_assigned, + protected_attribute_accessed, + protected_method_called, + protected_method_called_params, + protected_container_attribute, + protected_container_method, +]) +def test_protected_attribute_is_restricted( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Ensures that it is impossible to use protected attributes.""" + tree = parse_ast_tree(code) + + visitor = WrongAttributeVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [ProtectedAttributeViolation]) + + +@pytest.mark.parametrize('code', [ + protected_name_definition, + protected_name_attr_definition, + protected_self_attribute, + protected_self_method, + protected_cls_attribute, + protected_cls_method, + protected_attribute_definition, +]) +def test_protected_attribute_is_allowed( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Ensures that it is possible to use protected attributes.""" + tree = parse_ast_tree(code) + + visitor = WrongAttributeVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) diff --git a/tests/test_visitors/test_ast/test_strings/test_formatted_string.py b/tests/test_visitors/test_ast/test_builtins/test_formatted_string.py similarity index 84% rename from tests/test_visitors/test_ast/test_strings/test_formatted_string.py rename to tests/test_visitors/test_ast/test_builtins/test_formatted_string.py index 62444e8fa..f97805872 100644 --- a/tests/test_visitors/test_ast/test_strings/test_formatted_string.py +++ b/tests/test_visitors/test_ast/test_builtins/test_formatted_string.py @@ -5,11 +5,14 @@ from wemake_python_styleguide.violations.consistency import ( FormattedStringViolation, ) -from wemake_python_styleguide.visitors.ast.strings import WrongStringVisitor +from wemake_python_styleguide.visitors.ast.builtins import WrongStringVisitor regular_string = "'some value'" +binary_string = "b'binary'" +unicode_string = "u'unicode'" string_variable = "some = '123'" formated_string = "'x + y = {0}'.format(2)" +procent_format = "'x = %d' % 1" key_formated_string = "'x + y = {res}'.format(res=2)" variable_format = """ some = 'x = {0}' @@ -22,8 +25,11 @@ @pytest.mark.parametrize('code', [ regular_string, + binary_string, + unicode_string, string_variable, formated_string, + procent_format, key_formated_string, variable_format, ]) diff --git a/tests/test_visitors/test_ast/test_numbers/test_magic_numbers.py b/tests/test_visitors/test_ast/test_builtins/test_magic_numbers.py similarity index 95% rename from tests/test_visitors/test_ast/test_numbers/test_magic_numbers.py rename to tests/test_visitors/test_ast/test_builtins/test_magic_numbers.py index 1e89f0b34..77c68a280 100644 --- a/tests/test_visitors/test_ast/test_numbers/test_magic_numbers.py +++ b/tests/test_visitors/test_ast/test_builtins/test_magic_numbers.py @@ -2,13 +2,11 @@ import pytest +from wemake_python_styleguide.constants import MAGIC_NUMBERS_WHITELIST from wemake_python_styleguide.violations.best_practices import ( MagicNumberViolation, ) -from wemake_python_styleguide.visitors.ast.numbers import ( - MAGIC_NUMBERS_WHITELIST, - MagicNumberVisitor, -) +from wemake_python_styleguide.visitors.ast.builtins import MagicNumberVisitor # Correct usages: diff --git a/tests/test_visitors/test_ast/test_classes/test_yield_inside_init.py b/tests/test_visitors/test_ast/test_classes/test_yield_inside_init.py index 73437bc01..8271f9534 100644 --- a/tests/test_visitors/test_ast/test_classes/test_yield_inside_init.py +++ b/tests/test_visitors/test_ast/test_classes/test_yield_inside_init.py @@ -19,6 +19,18 @@ def __init__(self, *args, **kwargs): yield self """ +regular_method_with_yield = """ +class ModuleMembersVisitor(object): + def method(self, *args, **kwargs): + yield self +""" + +iter_with_yield = """ +class ModuleMembersVisitor(object): + def __iter__(self, *args, **kwargs): + yield self +""" + async_init_without_yield = """ class ModuleMembersVisitor(object): async def __init__(self, *args, **kwargs): @@ -54,6 +66,8 @@ def test_init_generator( @pytest.mark.parametrize('code', [ init_without_yield, async_init_without_yield, + regular_method_with_yield, + iter_with_yield, ]) def test_init_regular( assert_errors, diff --git a/tests/test_visitors/test_ast/test_complexity/test_jones/test_line_complexity.py b/tests/test_visitors/test_ast/test_complexity/test_jones/test_line_complexity.py index 20e807e9c..2601a909d 100644 --- a/tests/test_visitors/test_ast/test_complexity/test_jones/test_line_complexity.py +++ b/tests/test_visitors/test_ast/test_complexity/test_jones/test_line_complexity.py @@ -111,9 +111,9 @@ def test_same_complexity(parse_ast_tree, default_options): simple_visitor.run() typed_visitor.run() - assert len(simple_visitor._lines) == 1 - assert len(simple_visitor._lines[1]) == 3 - assert len(simple_visitor._lines[1]) == len(typed_visitor._lines[1]) + assert len(simple_visitor._lines) == 1 # noqa: Z441 + assert len(simple_visitor._lines[1]) == 3 # noqa: Z441 + assert len(typed_visitor._lines[1]) == 3 # noqa: Z441 @pytest.mark.parametrize('code, complexity', [ @@ -127,8 +127,8 @@ def test_exact_complexity(parse_ast_tree, default_options, code, complexity): visitor = JonesComplexityVisitor(default_options, tree=tree) visitor.run() - assert len(visitor._lines) == 1 - assert len(visitor._lines[1]) == complexity + assert len(visitor._lines) == 1 # noqa: Z441 + assert len(visitor._lines[1]) == complexity # noqa: Z441 @pytest.mark.parametrize('code, number_of_lines', [ @@ -147,4 +147,4 @@ def test_that_some_nodes_are_ignored( visitor = JonesComplexityVisitor(default_options, tree=tree) visitor.run() - assert len(visitor._lines) == number_of_lines + assert len(visitor._lines) == number_of_lines # noqa: Z441 diff --git a/tests/test_visitors/test_ast/test_imports/test_protected_import.py b/tests/test_visitors/test_ast/test_imports/test_protected_import.py index fde8ba115..1c09af6fd 100644 --- a/tests/test_visitors/test_ast/test_imports/test_protected_import.py +++ b/tests/test_visitors/test_ast/test_imports/test_protected_import.py @@ -2,7 +2,9 @@ import pytest -from wemake_python_styleguide.violations.naming import ProtectedModuleViolation +from wemake_python_styleguide.violations.best_practices import ( + ProtectedModuleViolation, +) from wemake_python_styleguide.visitors.ast.imports import WrongImportVisitor import_public = 'import public' @@ -19,11 +21,18 @@ import_from_public, import_from_public_path, ]) -def test_correct_import(assert_errors, parse_ast_tree, code, default_options): +def test_correct_import( + assert_errors, + parse_ast_tree, + code, + default_options, +): """Testing that correct imports are allowed.""" tree = parse_ast_tree(code) + visitor = WrongImportVisitor(default_options, tree=tree) visitor.run() + assert_errors(visitor, []) @@ -32,9 +41,16 @@ def test_correct_import(assert_errors, parse_ast_tree, code, default_options): import_from_protected, import_from_protected_path, ]) -def test_incorrect_import(assert_errors, parse_ast_tree, code, default_options): +def test_incorrect_import( + assert_errors, + parse_ast_tree, + code, + default_options, +): """Testing that imports from protected modules are restricted.""" tree = parse_ast_tree(code) + visitor = WrongImportVisitor(default_options, tree=tree) visitor.run() + assert_errors(visitor, [ProtectedModuleViolation]) diff --git a/tests/test_visitors/test_ast/test_keywords/test_loops.py b/tests/test_visitors/test_ast/test_keywords/test_loops.py index 23c6abcfa..529afcc7e 100644 --- a/tests/test_visitors/test_ast/test_keywords/test_loops.py +++ b/tests/test_visitors/test_ast/test_keywords/test_loops.py @@ -7,34 +7,115 @@ ) from wemake_python_styleguide.visitors.ast.keywords import WrongForElseVisitor -right_else_in_for_loop = """ +wrong_else_in_for_loop = """ for x in '123': ... else: ... """ -wrong_else_in_for_loop = """ +wrong_nested_else_in_for_loop = """ +for letters in ['abc', 'zxc', 'rrd']: + for x in letters: + ... + else: + ... +""" + +wrong_nested_for_with_break = """ +for letters in ['abc', 'zxc', 'rrd']: + for x in letters: + break +else: + ... +""" + +wrong_nested_while_with_break = """ +for letters in ['abc', 'zxc', 'rrd']: + while 'a' in letters: + break +else: + ... +""" + +wrong_multiple_breaks = """ +for x in 'zzz': + for i in range(10): + if i > 1: + break + else: + break +else: + ... +""" + +# Correct: + +right_else_in_for_loop = """ for x in '123': break else: - ... + ... +""" + +right_multiple_breaks = """ +for x in 'xxx': + for i in range(10): + if i > 1: + break + break +else: + ... +""" + +right_multiple_nested_for_with_break = """ +for letters in ['abc', 'zxc', 'rrd']: + for x in letters: + break + + for y in letters: + break + + while letters: + break +else: + ... +""" + +right_nested_break_in_for_loop = """ +for x in 'nnn': + if x == '1': + break +else: + ... """ check_nested_if_else = """ -for x in '123': +for x in '000': if x: ... else: ... """ +while_with_break = """ +while x > 2: + break +""" + @pytest.mark.parametrize('code', [ wrong_else_in_for_loop, + wrong_nested_else_in_for_loop, + wrong_nested_for_with_break, + wrong_nested_while_with_break, + wrong_multiple_breaks, ]) def test_wrong_else_in_for_loop( - assert_errors, parse_ast_tree, code, default_options, + assert_errors, + parse_ast_tree, + code, + default_options, ): """Violations are raised when else with break statement.""" tree = parse_ast_tree(code) @@ -47,10 +128,17 @@ def test_wrong_else_in_for_loop( @pytest.mark.parametrize('code', [ right_else_in_for_loop, + right_nested_break_in_for_loop, + right_multiple_nested_for_with_break, + right_multiple_breaks, check_nested_if_else, + while_with_break, ]) def test_correct_else_in_for_loop( - assert_errors, parse_ast_tree, code, default_options, + assert_errors, + parse_ast_tree, + code, + default_options, ): """Violations are not raised when else without break statement.""" tree = parse_ast_tree(code) diff --git a/wemake_python_styleguide/constants.py b/wemake_python_styleguide/constants.py index 9e0398365..aa49fbc16 100644 --- a/wemake_python_styleguide/constants.py +++ b/wemake_python_styleguide/constants.py @@ -80,7 +80,7 @@ 'objs', 'some', - # Confusables: + # Confuseables: 'no', 'true', 'false', @@ -160,6 +160,9 @@ # Used to specify as a placeholder for `__init__`: INIT: Final = '__init__' +# Allowed magic number modulo: +NON_MAGIC_MODULO: Final = 10 + # Used to specify a pattern which checks variables and modules for underscored # numbers in their names: UNDERSCORED_NUMBER_PATTERN: Final = re.compile(r'.*\D_\d(\D|$)') diff --git a/wemake_python_styleguide/logics/variables/access.py b/wemake_python_styleguide/logics/variables/access.py index 11b461e2f..312088380 100644 --- a/wemake_python_styleguide/logics/variables/access.py +++ b/wemake_python_styleguide/logics/variables/access.py @@ -1,15 +1,10 @@ # -*- coding: utf-8 -*- -from typing import Optional - -def is_private_variable(name: Optional[str]) -> bool: +def is_private_variable(name: str) -> bool: """ Checks if variable has private name pattern. - >>> is_private_variable(None) - False - >>> is_private_variable('regular') False @@ -23,9 +18,7 @@ def is_private_variable(name: Optional[str]) -> bool: False """ - return ( - name is not None and name.startswith('__') and not name.endswith('__') - ) + return name.startswith('__') and not name.endswith('__') def is_protected_variable(name: str) -> bool: diff --git a/wemake_python_styleguide/logics/variables/name_nodes.py b/wemake_python_styleguide/logics/variables/name_nodes.py index e4a5f7913..ff93a8827 100644 --- a/wemake_python_styleguide/logics/variables/name_nodes.py +++ b/wemake_python_styleguide/logics/variables/name_nodes.py @@ -13,7 +13,7 @@ def is_same_variable(left: ast.AST, right: ast.AST) -> bool: def get_assigned_name(node: ast.AST) -> Optional[str]: """ - Returns variable names for node that are just assigned. + Returns variable names for node that is just assigned. Returns ``None`` for nodes that are used in a different manner. """ diff --git a/wemake_python_styleguide/logics/variables/naming.py b/wemake_python_styleguide/logics/variables/naming.py index 38fed6cab..6965eb2e5 100644 --- a/wemake_python_styleguide/logics/variables/naming.py +++ b/wemake_python_styleguide/logics/variables/naming.py @@ -1,9 +1,9 @@ # -*- coding: utf-8 -*- -from typing import Iterable, Optional +from typing import Iterable from wemake_python_styleguide import constants -from wemake_python_styleguide.options.defaults import MIN_VARIABLE_LENGTH +from wemake_python_styleguide.options import defaults def is_wrong_variable_name(name: str, to_check: Iterable[str]) -> bool: @@ -40,7 +40,7 @@ def is_wrong_variable_name(name: str, to_check: Iterable[str]) -> bool: return False -def is_upper_case_name(name: Optional[str]) -> bool: +def is_upper_case_name(name: str) -> bool: """ Checks that attribute name has no upper-case letters. @@ -65,16 +65,13 @@ def is_upper_case_name(name: Optional[str]) -> bool: >>> is_upper_case_name('__variable_v2') False - >>> is_upper_case_name(None) - False - """ - return name is not None and any(character.isupper() for character in name) + return any(character.isupper() for character in name) def is_too_short_variable_name( - name: Optional[str], - min_length: int = MIN_VARIABLE_LENGTH, + name: str, + min_length: int = defaults.MIN_VARIABLE_LENGTH, ) -> bool: """ Checks for too short variable names. @@ -82,9 +79,6 @@ def is_too_short_variable_name( >>> is_too_short_variable_name('test') False - >>> is_too_short_variable_name(None) - False - >>> is_too_short_variable_name('o') True @@ -98,37 +92,9 @@ def is_too_short_variable_name( False """ - if name is None: - return False - return name != constants.UNUSED_VARIABLE and len(name) < min_length -def is_private_variable(name: Optional[str]) -> bool: - """ - Checks if variable has private name pattern. - - >>> is_private_variable(None) - False - - >>> is_private_variable('regular') - False - - >>> is_private_variable('__private') - True - - >>> is_private_variable('_protected') - False - - >>> is_private_variable('__magic__') - False - - """ - return ( - name is not None and name.startswith('__') and not name.endswith('__') - ) - - def is_variable_name_with_underscored_number(name: str) -> bool: """ Checks for variable names with underscored number. @@ -136,9 +102,6 @@ def is_variable_name_with_underscored_number(name: str) -> bool: >>> is_variable_name_with_underscored_number('star_wars_episode2') False - >>> is_variable_name_with_underscored_number(None) - False - >>> is_variable_name_with_underscored_number('come2_me') False @@ -159,7 +122,7 @@ def is_variable_name_with_underscored_number(name: str) -> bool: """ pattern = constants.UNDERSCORED_NUMBER_PATTERN - return name is not None and pattern.match(name) is not None + return pattern.match(name) is not None def is_variable_name_contains_consecutive_underscores(name: str) -> bool: @@ -175,9 +138,6 @@ def is_variable_name_contains_consecutive_underscores(name: str) -> bool: >>> is_variable_name_contains_consecutive_underscores('__private') False - >>> is_variable_name_contains_consecutive_underscores(None) - False - >>> is_variable_name_contains_consecutive_underscores('name') False @@ -188,12 +148,6 @@ def is_variable_name_contains_consecutive_underscores(name: str) -> bool: True """ - if name is None: - return False - - if name.endswith('__') and name.startswith('__'): - return False - if name.startswith('__'): return False diff --git a/wemake_python_styleguide/violations/best_practices.py b/wemake_python_styleguide/violations/best_practices.py index 2c2176b6d..15b354bca 100644 --- a/wemake_python_styleguide/violations/best_practices.py +++ b/wemake_python_styleguide/violations/best_practices.py @@ -39,6 +39,8 @@ RedundantFinallyViolation ReassigningVariableToItselfViolation YieldInsideInitViolation + ProtectedModuleViolation + ProtectedAttributeViolation Comments -------- @@ -74,6 +76,8 @@ .. autoclass:: RedundantFinallyViolation .. autoclass:: ReassigningVariableToItselfViolation .. autoclass:: YieldInsideInitViolation +.. autoclass:: ProtectedModuleViolation +.. autoclass:: ProtectedAttributeViolation """ @@ -689,9 +693,11 @@ def some(): @final class RedundantForElseViolation(ASTViolation): """ - Forbids to use ``else`` with ``break`` in ``for`` loop. + Forbids to use ``else`` without ``break`` in ``for`` loop. Reasoning: + When there's no ``break`` keyword in ``for`` body it means + that ``else`` will always be called. This rule will reduce complexity, improve readability, and protect from possible errors. @@ -699,6 +705,21 @@ class RedundantForElseViolation(ASTViolation): Refactor your ``else`` case logic to be inside the ``for`` body. + Example:: + + # Correct: + for letter in 'abc': + if letter == 'b': + break + else: + print('"b" is not found') + + # Wrong: + for letter in 'abc': + print(letter) + else: + print('"b" is not found') + .. versionadded:: 0.3.0 Note: @@ -707,7 +728,7 @@ class RedundantForElseViolation(ASTViolation): """ #: Error message shown to the user. - error_template = 'Found `else` in `for` loop with `break`' + error_template = 'Found `else` in `for` loop without `break`' code = 436 @@ -811,3 +832,84 @@ def __init__(self): #: Error message shown to the user. error_template = 'Found `yield` inside `__init__`' code = 439 + + +@final +class ProtectedModuleViolation(ASTViolation): + """ + Forbids to ``import`` protected modules. + + Reasoning: + When importing protected modules we break a contract + that authors of this module enforce. + This way we are not respecting encapsulation and it may break + our code at any moment. + + Solution: + Do not import anything from protected modules. + Respect the encapsulation. + + Example:: + + # Correct: + from some.public.module import FooClass + + # Wrong: + import _compat + from some._protected.module import BarClass + from some.module import _protected + + .. versionadded:: 0.3.0 + + Note: + Returns Z440 as error code + + """ + + #: Error message shown to the user. + error_template = 'Found protected module import "{0}"' + code = 440 + + +@final +class ProtectedAttributeViolation(ASTViolation): + """ + Forbids to use protected attributes and methods. + + Reasoning: + When using protected attributes and method we break a contract + that authors of this class enforce. + This way we are not respecting encapsulation and it may break + our code at any moment. + + Solution: + Do not use protected attributes and methods. + Respect the encapsulation. + + Example:: + + # Correct: + self._protected = 1 + cls._hidden_method() + some.public() + + # Wrong: + print(some._protected) + instance._hidden() + self.container._internal = 10 + + Note, that it is possible to use protected attributes with ``self`` + and ``cls`` as base names. We allow this so you can create and use + protected attributes and methods inside the class context. + This is how protected attributes should be used. + + .. versionadded:: 0.3.0 + + Note: + Returns Z441 as error code + + """ + + #: Error message shown to the user. + error_template = 'Found protected attribute usage "{0}"' + code = 441 diff --git a/wemake_python_styleguide/violations/naming.py b/wemake_python_styleguide/violations/naming.py index 2b259a103..079b71841 100644 --- a/wemake_python_styleguide/violations/naming.py +++ b/wemake_python_styleguide/violations/naming.py @@ -22,11 +22,12 @@ - Use names of an appropriate length: not too short, not too long - Protected members should use underscore as the first char - Private names are not allowed +- Do not use consecutive underscores - When writing abbreviations in ``UpperCase`` capitalize all letters: ``HTTPAddress`` - When writting abbreviations in ``snake_case`` use lowercase: ``http_address`` - When writting numbers in ``snake_case`` - do not use extra ``_`` as in ``http2_protocol`` + do not use extra ``_`` before numbers as in ``http2_protocol`` Packages ~~~~~~~~ @@ -74,7 +75,7 @@ - Python's ``*args`` and ``**kwargs`` should be default names when just passing these values to some other method/function - Unless you want to use these values in place, then name them explicitly -- Keyword-only arguments might be separated from other arguments with ``*`` +- Keyword-only arguments must be separated from other arguments with ``*`` Global (module level) variables ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -85,16 +86,15 @@ Variables ~~~~~~~~~ -- Variables should use ``snake_case`` +- Variables should use ``snake_case`` with no exceptions - When some variable is unused it should be prefixed with an underscore -- Variables should not contain more than one consecutive underscore Type aliases ~~~~~~~~~~~~ - Should use ``UpperCase`` as real classes - Should not contain word ``type`` in its name -- Generic types should be called ``TT`` or ``KK`` or ``VV`` +- Generic types should be called ``TT`` or ``KT`` or ``VT`` - Covariant and contravariant types should be marked with ``Cov`` and ``Contra`` suffixes - In this case one letter can be dropped: ``TCov`` and ``KContra`` @@ -119,7 +119,6 @@ UnderScoredNumberNameViolation UpperCaseAttributeViolation ConsecutiveUnderscoresInNameViolation - ProtectedModuleViolation Module names @@ -130,7 +129,6 @@ .. autoclass:: TooShortModuleNameViolation .. autoclass:: WrongModuleNameUnderscoresViolation .. autoclass:: WrongModuleNamePatternViolation -.. autoclass:: ProtectedModuleViolation Variable names -------------- @@ -571,36 +569,3 @@ class ConsecutiveUnderscoresInNameViolation(ASTViolation): error_template = 'Found consecutive underscores in a variable "{0}"' code = 116 - - -@final -class ProtectedModuleViolation(ASTViolation): - """ - Forbids to import or import from protected module. - - Reasoning: - Import starting with one underscore is found. - - Solution: - Do not import from protected module. - Rename module name to be not protected. - - Example:: - - # Correct: - from some.public.module import FooClass - - # Wrong: - from some._protected.module import BarClass - from some.module import _protected - - .. versionadded:: 0.3.0 - - Note: - Returns Z117 as error code - """ - - #: Error message shown to the user - error_template = 'Found protected module import "{0}"' - - code = 117 diff --git a/wemake_python_styleguide/visitors/ast/attributes.py b/wemake_python_styleguide/visitors/ast/attributes.py new file mode 100644 index 000000000..76d4fdbc1 --- /dev/null +++ b/wemake_python_styleguide/visitors/ast/attributes.py @@ -0,0 +1,39 @@ +# -*- coding: utf-8 -*- + +import ast +from typing import ClassVar, FrozenSet + +from wemake_python_styleguide.logics.variables import access +from wemake_python_styleguide.types import final +from wemake_python_styleguide.violations.best_practices import ( + ProtectedAttributeViolation, +) +from wemake_python_styleguide.visitors.base import BaseNodeVisitor + + +@final +class WrongAttributeVisitor(BaseNodeVisitor): + """Ensures that attributes are used correctly.""" + + _allowed_to_use_protected: ClassVar[FrozenSet[str]] = frozenset(( + 'self', + 'cls', + )) + + def _check_protected_attribute(self, node: ast.Attribute) -> None: + if access.is_protected_variable(node.attr): + if isinstance(node.value, ast.Name): + if node.value.id in self._allowed_to_use_protected: + return + self.add_violation(ProtectedAttributeViolation(node)) + + def visit_Attribute(self, node: ast.Attribute) -> None: + """ + Checks the `Attribute` node. + + Raises: + ProtectedAttributeViolation + + """ + self._check_protected_attribute(node) + self.generic_visit(node) diff --git a/wemake_python_styleguide/visitors/ast/numbers.py b/wemake_python_styleguide/visitors/ast/builtins.py similarity index 73% rename from wemake_python_styleguide/visitors/ast/numbers.py rename to wemake_python_styleguide/visitors/ast/builtins.py index 4f80f90cd..9d323da57 100644 --- a/wemake_python_styleguide/visitors/ast/numbers.py +++ b/wemake_python_styleguide/visitors/ast/builtins.py @@ -3,14 +3,33 @@ import ast from typing import ClassVar, Optional -from wemake_python_styleguide.constants import MAGIC_NUMBERS_WHITELIST +from wemake_python_styleguide import constants from wemake_python_styleguide.types import AnyNodes, final from wemake_python_styleguide.violations.best_practices import ( MagicNumberViolation, ) +from wemake_python_styleguide.violations.consistency import ( + FormattedStringViolation, +) from wemake_python_styleguide.visitors.base import BaseNodeVisitor +@final +class WrongStringVisitor(BaseNodeVisitor): + """Restricts to use ``f`` strings.""" + + def visit_JoinedStr(self, node: ast.JoinedStr) -> None: + """ + Restricts to use ``f`` strings. + + Raises: + FormattedStringViolation + + """ + self.add_violation(FormattedStringViolation(node)) + self.generic_visit(node) + + @final class MagicNumberVisitor(BaseNodeVisitor): """Checks magic numbers used in the code.""" @@ -54,10 +73,10 @@ def _check_is_magic(self, node: ast.Num) -> None: if isinstance(parent, self._allowed_parents): return - if node.n in MAGIC_NUMBERS_WHITELIST: + if node.n in constants.MAGIC_NUMBERS_WHITELIST: return - if isinstance(node.n, int) and node.n <= 10: + if isinstance(node.n, int) and node.n <= constants.NON_MAGIC_MODULO: return self.add_violation(MagicNumberViolation(node, text=str(node.n))) diff --git a/wemake_python_styleguide/visitors/ast/imports.py b/wemake_python_styleguide/visitors/ast/imports.py index 3b345e312..8d8ab397a 100644 --- a/wemake_python_styleguide/visitors/ast/imports.py +++ b/wemake_python_styleguide/visitors/ast/imports.py @@ -12,15 +12,13 @@ from wemake_python_styleguide.violations.best_practices import ( FutureImportViolation, NestedImportViolation, + ProtectedModuleViolation, ) from wemake_python_styleguide.violations.consistency import ( DottedRawImportViolation, LocalFolderImportViolation, ) -from wemake_python_styleguide.violations.naming import ( - ProtectedModuleViolation, - SameAliasImportViolation, -) +from wemake_python_styleguide.violations.naming import SameAliasImportViolation from wemake_python_styleguide.visitors.base import BaseNodeVisitor ErrorCallback = Callable[[BaseViolation], None] diff --git a/wemake_python_styleguide/visitors/ast/keywords.py b/wemake_python_styleguide/visitors/ast/keywords.py index 6c63a64c7..22696ef49 100644 --- a/wemake_python_styleguide/visitors/ast/keywords.py +++ b/wemake_python_styleguide/visitors/ast/keywords.py @@ -2,7 +2,7 @@ import ast from collections import defaultdict -from typing import ClassVar, DefaultDict +from typing import ClassVar, DefaultDict, Optional, Union from wemake_python_styleguide.types import AnyNodes, final from wemake_python_styleguide.violations.best_practices import ( @@ -20,6 +20,8 @@ ) from wemake_python_styleguide.visitors.base import BaseNodeVisitor +AnyLoop = Union[ast.For, ast.While] + @final class _ComprehensionComplexityCounter(object): @@ -130,20 +132,43 @@ def visit_comprehension(self, node: ast.comprehension) -> None: @final class WrongForElseVisitor(BaseNodeVisitor): - """Responsible for restricting else in for loops with break.""" + """Responsible for restricting `else` in `for` loops without `break`.""" + + def _does_loop_contain_node( + self, + loop: Optional[AnyLoop], + to_check: ast.Break, + ) -> bool: + if loop is None: + return False + + for inner_node in ast.walk(loop): + if to_check is inner_node: + return True + return False + + def _has_break(self, node: ast.For) -> bool: + closest_loop = None + + for subnode in ast.walk(node): + if isinstance(subnode, (ast.For, ast.While)): + if subnode is not node: + closest_loop = subnode + + if isinstance(subnode, ast.Break): + is_nested_break = self._does_loop_contain_node( + closest_loop, subnode, + ) + if not is_nested_break: + return True + return False def _check_for_needs_else(self, node: ast.For) -> None: - break_in_for_loop = False - - for condition in ast.walk(node): - if isinstance(condition, ast.Break): - break_in_for_loop = True - - if node.orelse and break_in_for_loop: + if node.orelse and not self._has_break(node): self.add_violation(RedundantForElseViolation(node=node)) def visit_For(self, node: ast.For) -> None: - """Used for find else block in for loops with break.""" + """Used for find `else` block in `for` loops without `break`.""" self._check_for_needs_else(node) self.generic_visit(node) diff --git a/wemake_python_styleguide/visitors/ast/strings.py b/wemake_python_styleguide/visitors/ast/strings.py deleted file mode 100644 index d17d63177..000000000 --- a/wemake_python_styleguide/visitors/ast/strings.py +++ /dev/null @@ -1,23 +0,0 @@ -# -*- coding: utf-8 -*- - -from wemake_python_styleguide.types import final -from wemake_python_styleguide.violations.consistency import ( - FormattedStringViolation, -) -from wemake_python_styleguide.visitors.base import BaseNodeVisitor - - -@final -class WrongStringVisitor(BaseNodeVisitor): - """Restricts to use ``f`` strings.""" - - def visit_JoinedStr(self, node) -> None: # type is not defined in ast yet - """ - Restricts to use ``f`` strings. - - Raises: - FormattedStringViolation - - """ - self.add_violation(FormattedStringViolation(node)) - self.generic_visit(node) diff --git a/wemake_python_styleguide/visitors/presets/general.py b/wemake_python_styleguide/visitors/presets/general.py index 0184ca3ed..9435ab533 100644 --- a/wemake_python_styleguide/visitors/presets/general.py +++ b/wemake_python_styleguide/visitors/presets/general.py @@ -1,10 +1,11 @@ # -*- coding: utf-8 -*- from wemake_python_styleguide.visitors.ast import ( + attributes, + builtins, comparisons, keywords, naming, - numbers, ) from wemake_python_styleguide.visitors.ast.classes import WrongClassVisitor from wemake_python_styleguide.visitors.ast.functions import ( @@ -14,7 +15,6 @@ from wemake_python_styleguide.visitors.ast.modules import ( EmptyModuleContentsVisitor, ) -from wemake_python_styleguide.visitors.ast.strings import WrongStringVisitor from wemake_python_styleguide.visitors.filenames.wrong_module_name import ( WrongModuleNameVisitor, ) @@ -29,6 +29,8 @@ keywords.WrongTryFinallyVisitor, keywords.WrongExceptionTypeVisitor, + attributes.WrongAttributeVisitor, + WrongFunctionCallVisitor, WrongImportVisitor, @@ -36,8 +38,8 @@ naming.WrongModuleMetadataVisitor, naming.WrongVariableAssignmentVisitor, - numbers.MagicNumberVisitor, - WrongStringVisitor, + builtins.MagicNumberVisitor, + builtins.WrongStringVisitor, comparisons.WrongConditionalVisitor, comparisons.ComparisonSanityVisitor,