Remove ImplicitTernaryViolation - WPS332 (#1099) (#1126)

We are removing this rule because we found out that it was not
applicable to all cases.

This is the first time we remove an existing rule, so a mechanism to
keep track of them and satisfy test_no_holes was needed. The implemented
mechanism involves just adding the deprecated code to the
DEPRECATED_CODES tuple, which must be defined on the file where the
violation class lived. This variable is not defined at all if there are
no deprecated codes on the file.

CHANGELOG.md

@@ -11,13 +11,15 @@ Semantic versioning in our case means:
 
 ## 0.14.0 WIP
 
-### Features 
+### Features
 
 - Extracts new violation - WPS450 from WPS436 #1118
 - Adds domain names options, that are used to create variable names' blacklist #1106
 
 ### Bugfixes
 
+- Remove ImplicitTernaryViolation - WPS332 #1099
+
 ### Misc
 
 - Adds `local-partial-types` to mypy config

setup.cfg

@@ -55,6 +55,8 @@ per-file-ignores =
   tests/*.py: S101, S105, S404, S603, S607, WPS211, WPS226
   # Docs can have the configuration they need:
   docs/conf.py: WPS407
+  # Pytest fixtures
+  tests/plugins/*.py: WPS442
 
 
 [isort]

tests/fixtures/noqa.py

@@ -313,8 +313,6 @@ def some_other_function():
     some_value = 1
     return some_value  # noqa: WPS331
 
-some_cond = cond() and 1 or None  # noqa: WPS332
-
 print(one > two and two > three)  # noqa: WPS333
 
 print(biggesst > middle >= smallest)  # noqa: WPS334

tests/plugins/violations.py

@@ -22,6 +22,18 @@
     TokenizeViolation,
 )
 
+VIOLATIONS_MODULES = (
+    system,
+    naming,
+    complexity,
+    consistency,
+    best_practices,
+    refactoring,
+    oop,
+)
+
+_SESSION_SCOPE = 'session'
+
 
 def _is_violation_class(cls) -> bool:
     base_classes = {
@@ -38,18 +50,8 @@ def _is_violation_class(cls) -> bool:
 
 
 def _load_all_violation_classes():
-    modules = [
-        system,
-        naming,
-        complexity,
-        consistency,
-        best_practices,
-        refactoring,
-        oop,
-    ]
-
     classes = {}
-    for module in modules:
+    for module in VIOLATIONS_MODULES:
         classes_names_list = inspect.getmembers(module, _is_violation_class)
         only_classes = map(itemgetter(1), classes_names_list)
         classes.update({
@@ -58,7 +60,7 @@ def _load_all_violation_classes():
     return classes
 
 
-@pytest.fixture(scope='session')
+@pytest.fixture(scope=_SESSION_SCOPE)
 def all_violations():
     """Loads all violations from the package and creates a flat list."""
     classes = _load_all_violation_classes()
@@ -68,7 +70,7 @@ def all_violations():
     return all_errors_container
 
 
-@pytest.fixture(scope='session')
+@pytest.fixture(scope=_SESSION_SCOPE)
 def all_controlled_violations():
     """Loads all violations which may be tweaked using `i_control_code`."""
     classes = _load_all_violation_classes()
@@ -80,7 +82,33 @@ def all_controlled_violations():
     return controlled_errors_container
 
 
-@pytest.fixture(scope='session')
+@pytest.fixture(scope=_SESSION_SCOPE)
 def all_module_violations():
     """Loads all violations from the package."""
     return _load_all_violation_classes()
+
+
+@pytest.fixture(scope=_SESSION_SCOPE)
+def all_deprecated_violation_codes():
+    """Loads all deprecated codes from the package."""
+    codes = {}
+    for module in VIOLATIONS_MODULES:
+        module_deprecated_codes = getattr(module, 'DEPRECATED_CODES', ())
+        codes[module] = sorted(module_deprecated_codes)
+    return codes
+
+
+@pytest.fixture(scope=_SESSION_SCOPE)
+def all_violation_codes(all_module_violations, all_deprecated_violation_codes):
+    """Loads all codes and their violation classes from the package."""
+    all_codes = {}
+    for module in all_module_violations.keys():
+        violation_codes = {
+            violation.code: violation
+            for violation in all_module_violations[module]
+        }
+        deprecated_codes = {
+            code: None for code in all_deprecated_violation_codes[module]
+        }
+        all_codes[module] = {**violation_codes, **deprecated_codes}
+    return all_codes

tests/test_checker/test_noqa.py

@@ -110,7 +110,6 @@
     'WPS329': 1,
     'WPS330': 1,
     'WPS331': 1,
-    'WPS332': 1,
     'WPS333': 1,
     'WPS334': 1,
     'WPS335': 1,

tests/test_violations/test_codes.py

@@ -32,13 +32,16 @@ def test_violations_start_zero(all_module_violations):
         assert starting_code == index * 100
 
 
-def test_no_holes(all_module_violations):
+def test_no_holes(all_violation_codes):
     """Ensures that there are no off-by-one errors."""
-    for module in all_module_violations.keys():
+    for module_codes in all_violation_codes.values():
         previous_code = None
-
-        for violation_class in all_module_violations[module]:
+        for code in sorted(module_codes.keys()):
             if previous_code is not None:
-                diff = violation_class.code - previous_code
-                assert diff == 1 or diff > 2, violation_class.__qualname__
-            previous_code = violation_class.code
+                diff = code - previous_code
+                assertion_name = (
+                    module_codes[code].__qualname__
+                    if module_codes[code] else 'DEPRECATED CODE'
+                )
+                assert diff == 1 or diff > 2, assertion_name
+            previous_code = code

wemake_python_styleguide/violations/consistency.py

@@ -57,7 +57,6 @@
    UselessExceptCaseViolation
    UselessOperatorsViolation
    InconsistentReturnVariableViolation
-   ImplicitTernaryViolation
    ImplicitComplexCompareViolation
    ReversedComplexCompareViolation
    WrongLoopIterTypeViolation
@@ -118,7 +117,6 @@
 .. autoclass:: UselessExceptCaseViolation
 .. autoclass:: UselessOperatorsViolation
 .. autoclass:: InconsistentReturnVariableViolation
-.. autoclass:: ImplicitTernaryViolation
 .. autoclass:: ImplicitComplexCompareViolation
 .. autoclass:: ReversedComplexCompareViolation
 .. autoclass:: WrongLoopIterTypeViolation
@@ -153,6 +151,8 @@
     TokenizeViolation,
 )
 
+DEPRECATED_CODES = (332,)
+
 
 @final
 class LocalFolderImportViolation(ASTViolation):
@@ -1302,35 +1302,6 @@ def some_function():
     code = 331
 
 
-@final
-class ImplicitTernaryViolation(ASTViolation):
-    """
-    Forbids to have implicit ternary expressions.
-
-    Reasoning:
-        This is done for consistency and readability reasons.
-        We believe that explicit ternary is better for readability.
-        This also allows you to identify hidden conditionals in your code.
-
-    Solution:
-        Refactor to use explicit ternary, or ``if`` condition.
-
-    Example::
-
-        # Correct:
-        some = one if cond() else two
-
-        # Wrong:
-        some = cond() and one or two
-
-    .. versionadded:: 0.10.0
-
-    """
-
-    code = 332
-    error_template = 'Found implicit ternary expression'
-
-
 @final
 class ImplicitComplexCompareViolation(ASTViolation):
     """

wemake_python_styleguide/visitors/ast/conditions.py

@@ -10,14 +10,12 @@
 from wemake_python_styleguide.logic import ifs, operators, source
 from wemake_python_styleguide.logic.compares import CompareBounds
 from wemake_python_styleguide.logic.functions import given_function_called
-from wemake_python_styleguide.logic.nodes import get_parent
 from wemake_python_styleguide.types import AnyIf, AnyNodes
 from wemake_python_styleguide.violations.best_practices import (
     SameElementsInConditionViolation,
 )
 from wemake_python_styleguide.violations.consistency import (
     ImplicitComplexCompareViolation,
-    ImplicitTernaryViolation,
     MultilineConditionsViolation,
 )
 from wemake_python_styleguide.violations.refactoring import (
@@ -225,13 +223,11 @@ def visit_BoolOp(self, node: ast.BoolOp) -> None:
         Checks that ``and`` and ``or`` do not form implicit anti-patterns.
 
         Raises:
-            ImplicitTernaryViolation
             ImplicitComplexCompareViolation
             ImplicitInConditionViolation
 
         """
         self._check_implicit_in(node)
-        self._check_implicit_ternary(node)
         self._check_implicit_complex_compare(node)
         self.generic_visit(node)
 
@@ -257,28 +253,6 @@ def _check_implicit_in(self, node: ast.BoolOp) -> None:
                 ImplicitInConditionViolation(node, text=duplicate),
             )
 
-    def _check_implicit_ternary(self, node: ast.BoolOp) -> None:
-        if isinstance(get_parent(node), ast.BoolOp):
-            return
-
-        if not isinstance(node.op, ast.Or):
-            return
-
-        if len(node.values) != 2:
-            return
-
-        if not isinstance(node.values[0], ast.BoolOp):
-            return
-
-        is_implicit_ternary = (
-            len(node.values[0].values) == 2 and
-            not isinstance(node.values[1], ast.BoolOp) and
-            isinstance(node.values[0].op, ast.And) and
-            not isinstance(node.values[0].values[1], ast.BoolOp)
-        )
-        if is_implicit_ternary:
-            self.add_violation(ImplicitTernaryViolation(node))
-
     def _check_implicit_complex_compare(self, node: ast.BoolOp) -> None:
         if not isinstance(node.op, ast.And):
             return