Repository: MartinThoma/flake8-simplify (python)

Issue:
[New Rule] Use keyword-arguments instead of positional arguments for boolean values
## Explanation

Positional boolean arguments are typically hard to understand, because they can represent so many things. Having a keyword-argument gives the reader the keyword name as context.

## Example

```python
import zipfile

# Bad
fp = zipfile.ZipFile("filename", "w", zipfile.ZIP_DEFLATED, False)

# Good
fp = zipfile.ZipFile("filename", "w", zipfile.ZIP_DEFLATED, allowZip64=False)
```


Pull request description:
Add SIM122/SIM123 to check for magic booleans/numbers
* SIM902
* SIM903

Closes #43 

Patch:
diff --git a/README.md b/README.md
index 5703aa7..ff3bd22 100644
--- a/README.md
+++ b/README.md
@@ -51,6 +51,7 @@ Python-specific rules:
 * `SIM124`: Reserved for [SIM904](#sim904) once it's stable
 * `SIM125`: Reserved for [SIM905](#sim905) once it's stable
 * `SIM126`: Reserved for [SIM906](#sim906) once it's stable
+* `SIM127`: Reserved for [SIM907](#sim907) once it's stable
 
 Simplifying Comparations:
 
@@ -83,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:**
 
@@ -95,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))
@@ -513,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.
@@ -559,3 +593,19 @@ os.path.join(a, os.path.join(b, c))
 # Good
 os.path.join(a, b, c)
 ```
+
+### SIM907
+
+This rule will be renamed to `SIM127` after its 6-month trial period is over.
+Please report any issues you encounter with this rule!
+
+```python
+# Bad
+def foo(a: Union[int, None]) -> Union[int, None]:
+    return a
+
+
+# Good
+def foo(a: Optional[int]) -> Optional[int]:
+    return a
+```
diff --git a/flake8_simplify/__init__.py b/flake8_simplify/__init__.py
index f8b9558..d2e55f6 100644
--- a/flake8_simplify/__init__.py
+++ b/flake8_simplify/__init__.py
@@ -17,6 +17,8 @@ from flake8_simplify.rules.ast_bool_op import (
 from flake8_simplify.rules.ast_call import (
     get_sim115,
     get_sim901,
+    get_sim902,
+    get_sim903,
     get_sim905,
     get_sim906,
 )
@@ -37,6 +39,7 @@ from flake8_simplify.rules.ast_if import (
     get_sim401,
 )
 from flake8_simplify.rules.ast_ifexp import get_sim210, get_sim211, get_sim212
+from flake8_simplify.rules.ast_subscript import get_sim907
 from flake8_simplify.rules.ast_try import get_sim105, get_sim107
 from flake8_simplify.rules.ast_unary_op import (
     get_sim201,
@@ -69,6 +72,8 @@ class Visitor(ast.NodeVisitor):
     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)
@@ -105,6 +110,10 @@ class Visitor(ast.NodeVisitor):
         self.errors += get_sim113(For(node))
         self.generic_visit(node)
 
+    def visit_Subscript(self, node: ast.Subscript) -> None:
+        self.errors += get_sim907(node)
+        self.generic_visit(node)
+
     def visit_Try(self, node: ast.Try) -> None:
         self.errors += get_sim105(node)
         self.errors += get_sim107(node)
diff --git a/flake8_simplify/rules/ast_call.py b/flake8_simplify/rules/ast_call.py
index b78c460..af9e8c2 100644
--- a/flake8_simplify/rules/ast_call.py
+++ b/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]] = []
diff --git a/flake8_simplify/rules/ast_subscript.py b/flake8_simplify/rules/ast_subscript.py
new file mode 100644
index 0000000..0d70941
--- /dev/null
+++ b/flake8_simplify/rules/ast_subscript.py
@@ -0,0 +1,59 @@
+# Core Library
+import ast
+from typing import List, Tuple
+
+# First party
+from flake8_simplify.constants import BOOL_CONST_TYPES
+from flake8_simplify.utils import to_source
+
+
+def get_sim907(node: ast.Subscript) -> List[Tuple[int, int, str]]:
+    """
+
+    Subscript(
+        value=Name(id='Union', ctx=Load()),
+        slice=Tuple(
+            elts=[
+                Name(id='int', ctx=Load()),
+                Name(id='str', ctx=Load()),
+                Constant(value=None, kind=None),
+            ],
+            ...
+        )
+    )
+    """
+    errors: List[Tuple[int, int, str]] = []
+
+    if not (isinstance(node.value, ast.Name) and node.value.id == "Union"):
+        return errors
+
+    if isinstance(node.slice, ast.Index) and isinstance(
+        node.slice.value, ast.Tuple  # type: ignore
+    ):
+        # Python 3.8
+        tuple_var = node.slice.value  # type: ignore
+    elif isinstance(node.slice, ast.Tuple):
+        # Python 3.9+
+        tuple_var = node.slice
+    else:
+        return errors
+
+    has_none = False
+    others = []
+    for elt in tuple_var.elts:  # type: ignore
+        if isinstance(elt, BOOL_CONST_TYPES) and elt.value is None:
+            has_none = True
+        else:
+            others.append(elt)
+
+    RULE = "SIM907 Use 'Optional[{type_}]' instead of '{original}'"
+    if len(others) == 1 and has_none:
+        type_ = to_source(others[0])
+        errors.append(
+            (
+                node.lineno,
+                node.col_offset,
+                RULE.format(type_=type_, original=to_source(node)),
+            )
+        )
+    return errors


Test patch:
diff --git a/tests/test_900_rules.py b/tests/test_900_rules.py
index f091ef7..074b22f 100644
--- a/tests/test_900_rules.py
+++ b/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",
     (
@@ -85,3 +171,13 @@ def test_sim905():
 def test_sim906(s, msg):
     results = _results(s)
     assert results == {msg}
+
+
+def test_sim907():
+    results = _results(
+        """def foo(a: Union[int, None]) -> bool:
+  return a"""
+    )
+    assert results == {
+        "1:11 SIM907 Use 'Optional[int]' instead of 'Union[int, None]'"
+    }