SIM122/SIM123: Added to check for magic booleans/numbers (#47)

Rules added:

* SIM902
* SIM903

Closes #43

README.md

@@ -84,6 +84,8 @@ General Code Style:
 * [`SIM103`](https://github.com/MartinThoma/flake8-simplify/issues/3): Return the boolean condition directly ([example](#SIM103))
 * [`SIM106`](https://github.com/MartinThoma/flake8-simplify/issues/8): Handle error-cases first ([example](#SIM106)). This rule was removed due to too many false-positives.
 * [`SIM112`](https://github.com/MartinThoma/flake8-simplify/issues/19): Use CAPITAL environment variables ([example](#SIM112))
+* `SIM122`: Reserved for SIM902 once it's stable
+* `SIM123`: Reserved for SIM903 once it's stable
 
 **Experimental rules:**
 
@@ -96,6 +98,8 @@ the code will change to another number.
 Current experimental rules:
 
 * `SIM901`: Use comparisons directly instead of wrapping them in a `bool(...)` call ([example](#SIM901))
+* `SIM902`: Use keyword-argument instead of magic boolean ([example](#SIM902))
+* `SIM903`: Use keyword-argument instead of magic number ([example](#SIM903))
 * `SIM904`: Assign values to dictionary directly at initialization ([example](#SIM904))
 * [`SIM905`](https://github.com/MartinThoma/flake8-simplify/issues/86): Split string directly if only constants are used ([example](#SIM905))
 * [`SIM906`](https://github.com/MartinThoma/flake8-simplify/issues/101): Merge nested os.path.join calls ([example](#SIM906))
@@ -514,6 +518,35 @@ bool(a == b)
 a == b
 ```
 
+### SIM902
+
+This rule will be renamed to `SIM122` after its 6-month trial period is over.
+
+```python
+# Bad
+foo(False)
+bar(True)
+
+# Good
+foo(verbose=False)
+bar(enable_magic=True)
+```
+
+### SIM903
+
+This rule will be renamed to `SIM123` after its 6-month trial period is over.
+Please report any issues you encounter with this rule!
+
+The trial period starts on 12th of February and will end on 12th of September 2022.
+
+```python
+# Bad
+foo(42, 1.234)
+
+# Good
+foo(the_answer=42, flux_compensation=1.234)
+```
+
 ### SIM904
 
 This rule will be renamed to `SIM224` after its 6-month trial period is over.

flake8_simplify/__init__.py

@@ -17,6 +17,8 @@
 from flake8_simplify.rules.ast_call import (
     get_sim115,
     get_sim901,
+    get_sim902,
+    get_sim903,
     get_sim905,
     get_sim906,
 )
@@ -70,6 +72,8 @@ def visit_Assign(self, node: ast.Assign) -> Any:
     def visit_Call(self, node: ast.Call) -> Any:
         self.errors += get_sim115(Call(node))
         self.errors += get_sim901(node)
+        self.errors += get_sim902(Call(node))
+        self.errors += get_sim903(Call(node))
         self.errors += get_sim905(node)
         self.errors += get_sim906(node)
         self.generic_visit(node)

flake8_simplify/rules/ast_call.py

@@ -92,6 +92,112 @@ def get_sim901(node: ast.Call) -> List[Tuple[int, int, str]]:
     return errors
 
 
+def get_sim902(node: Call) -> List[Tuple[int, int, str]]:
+    """Find bare boolean function arguments."""
+    SIM902 = (
+        "SIM902 Use keyword-argument instead of magic boolean for '{func}'"
+    )
+    errors: List[Tuple[int, int, str]] = []
+
+    if isinstance(node.func, ast.Attribute):
+        call_name = node.func.attr
+    elif isinstance(node.func, ast.Name):
+        call_name = node.func.id
+    else:
+        logger.debug(f"Unknown call type: {type(node.func)}")
+        return errors
+
+    nb_args = len(node.args)
+
+    if call_name in ["partial", "min", "max"] or call_name.startswith("_"):
+        return errors
+
+    has_bare_bool = any(
+        isinstance(call_arg, (ast.Constant, ast.NameConstant))
+        and (call_arg.value is True or call_arg.value is False)
+        for call_arg in node.args
+    )
+
+    is_setter = call_name.lower().startswith("set") and nb_args <= 2
+    is_exception = isinstance(node.func, ast.Attribute) and node.func.attr in [
+        "get"
+    ]
+    if has_bare_bool and not (is_exception or is_setter):
+        source = to_source(node)
+        errors.append(
+            (node.lineno, node.col_offset, SIM902.format(func=source))
+        )
+    return errors
+
+
+def get_sim903(node: Call) -> List[Tuple[int, int, str]]:
+    """Find bare numeric function arguments."""
+    SIM903 = "SIM903 Use keyword-argument instead of magic number for '{func}'"
+    acceptable_magic_numbers = (0, 1, 2)
+    errors: List[Tuple[int, int, str]] = []
+
+    if isinstance(node.func, ast.Attribute):
+        call_name = node.func.attr
+    elif isinstance(node.func, ast.Name):
+        call_name = node.func.id
+    else:
+        logger.debug(f"Unknown call type: {type(node.func)}")
+        return errors
+
+    nb_args = len(node.args)
+    if nb_args <= 1 or call_name.startswith("_"):
+        return errors
+
+    functions_any_arg = ["partial", "min", "max", "minimum", "maximum"]
+    functions_1_arg = ["sqrt", "sleep", "hideColumn"]
+    functions_2_args = [
+        "arange",
+        "uniform",
+        "zeros",
+        "percentile",
+        "setColumnWidth",
+        "float_power",
+        "power",
+        "pow",
+        "float_power",
+        "binomial",
+    ]
+    if any(
+        (
+            call_name in functions_any_arg,
+            call_name in functions_1_arg and nb_args == 1,
+            call_name in functions_2_args and nb_args == 2,
+            call_name in ["linspace"] and nb_args == 3,
+            "color" in call_name.lower() and nb_args in [3, 4],
+            "point" in call_name.lower() and nb_args in [2, 3],
+        )
+    ):
+        return errors
+
+    has_bare_int = any(
+        isinstance(call_arg, ast.Num)
+        and call_arg.n not in acceptable_magic_numbers
+        for call_arg in node.args
+    )
+
+    is_setter = call_name.lower().startswith("set") and nb_args <= 2
+    is_exception = isinstance(node.func, ast.Name) and node.func.id == "range"
+    is_exception = is_exception or (
+        isinstance(node.func, ast.Attribute)
+        and node.func.attr
+        in [
+            "get",
+            "insert",
+        ]
+    )
+    if has_bare_int and not (is_exception or is_setter):
+        source = to_source(node)
+        errors.append(
+            (node.lineno, node.col_offset, SIM903.format(func=source))
+        )
+    return errors
+
+
 def get_sim905(node: ast.Call) -> List[Tuple[int, int, str]]:
     RULE = "SIM905 Use '{expected}' instead of '{actual}'"
     errors: List[Tuple[int, int, str]] = []

tests/test_900_rules.py

@@ -10,6 +10,92 @@ def test_sim901():
     assert results == {"1:0 SIM901 Use 'a == b' instead of 'bool(a == b)'"}
 
 
+@pytest.mark.parametrize(
+    "s",
+    (
+        "foo(a, b, True)",
+        "set_foo(a, b, True)",
+    ),
+    ids=[
+        "basic",
+        "set_multiple",
+    ],
+)
+def test_sim902(s):
+    error_messages = _results(s)
+    assert any("SIM902" in error_message for error_message in error_messages)
+
+
+@pytest.mark.parametrize(
+    "s",
+    (
+        "foo(a, b, foo=True)",
+        "dict.get('foo', True)",
+        "set_visible(True)",
+        "line.set_visible(True)",
+        "partial(foo, True)",
+        "partial(foo, bar=True)",
+    ),
+    ids=[
+        "kw_arg_is_used",
+        "dict_get",
+        "boolean_setter_function",
+        "boolean_setter_method",
+        "partial_arg",
+        "partial_kwarg",
+    ],
+)
+def test_sim902_false_positive_check(s):
+    error_messages = _results(s)
+    for error_message in error_messages:
+        assert "SIM902" not in error_message
+
+
+@pytest.mark.parametrize(
+    "s",
+    ("foo(a, b, 123123)", "foo(a, b, 123.123)"),
+    ids=["int", "float"],
+)
+def test_sim903_true_positive_check(s):
+    error_messages = _results(s)
+    assert any("SIM903" in error_message for error_message in error_messages)
+
+
+@pytest.mark.parametrize(
+    "s",
+    (
+        "dict.get('foo', 123)",
+        "set_foo(1.23)",
+        "line.set_foo(1.23)",
+        "partial(foo, 1, 2, 3)",
+        "min(0.5, g_norm)",
+        "QColor(53, 53, 53, 128)",
+    ),
+    ids=[
+        "get_exception",
+        "set_function",
+        "set_method",
+        "partial",
+        "min",
+        "color",
+    ],
+)
+def test_sim903_false_positive_check(s):
+    error_messages = _results(s)
+    for error_message in error_messages:
+        assert "SIM903" not in error_message
+
+
+def test_sim903_insert_exception():
+    ret = _results("sys.path.insert(0, 'foo')")
+    assert ret == set()
+
+
+def test_sim903_range_exception():
+    ret = _results("range(42)")
+    assert ret == set()
+
+
 @pytest.mark.parametrize(
     "s",
     (