SIM908: Added (#52)

This will be renamed to SIM121 on 20th of September 2022.

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>

README.md

@@ -48,6 +48,7 @@ Python-specific rules:
 * [`SIM117`](https://github.com/MartinThoma/flake8-simplify/issues/35): Merge with-statements that use the same scope ([example](#SIM117))
 * [`SIM119`](https://github.com/MartinThoma/flake8-simplify/issues/37) ![](https://shields.io/badge/-legacyfix-inactive): Use dataclasses for data containers ([example](#SIM119))
 * `SIM120` ![](https://shields.io/badge/-legacyfix-inactive): Use 'class FooBar:' instead of 'class FooBar(object):' ([example](#SIM120))
+* `SIM121`: Reserved for [SIM908](#sim908) once it's stable
 * `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
@@ -357,18 +358,20 @@ A lot of projects use them:
 
 ### SIM120
 
-```
+```python
 # Bad
 class FooBar(object):
     ...
 
+
 # Good
 class FooBar:
     ...
 ```
 
 Both notations are equivalent in Python 3, but the second one is shorter.
 
+
 ### SIM201
 
 ```python
@@ -599,6 +602,8 @@ os.path.join(a, b, c)
 This rule will be renamed to `SIM127` after its 6-month trial period is over.
 Please report any issues you encounter with this rule!
 
+The trial period starts on 28th of March and will end on 28th of September 2022.
+
 ```python
 # Bad
 def foo(a: Union[int, None]) -> Union[int, None]:
@@ -609,3 +614,20 @@ def foo(a: Union[int, None]) -> Union[int, None]:
 def foo(a: Optional[int]) -> Optional[int]:
     return a
 ```
+
+### SIM908
+
+This rule will be renamed to `SIM121` after its 6-month trial period is over.
+Please report any issues you encounter with this rule!
+
+The trial period starts on 28th of March and will end on 28th of September 2022.
+
+```python
+# Bad
+name = "some_default"
+if "some_key" in some_dict:
+    name = some_dict["some_key"]
+
+# Good
+name = some_dict.get("some_key", "some_default")
+```

flake8_simplify/__init__.py

@@ -37,6 +37,7 @@
     get_sim114,
     get_sim116,
     get_sim401,
+    get_sim908,
 )
 from flake8_simplify.rules.ast_ifexp import get_sim210, get_sim211, get_sim212
 from flake8_simplify.rules.ast_subscript import get_sim907
@@ -101,6 +102,7 @@ def visit_If(self, node: ast.If) -> None:
         self.errors += get_sim108(node)
         self.errors += get_sim114(node)
         self.errors += get_sim116(node)
+        self.errors += get_sim908(node)
         self.errors += get_sim401(node)
         self.generic_visit(node)
 

flake8_simplify/rules/ast_if.py

@@ -291,6 +291,45 @@ def get_sim116(node: ast.If) -> List[Tuple[int, int, str]]:
     return errors
 
 
+def get_sim908(node: ast.If) -> List[Tuple[int, int, str]]:
+    """
+    Get all if-blocks which only check if a key is in a dictionary.
+    """
+    RULE = (
+        "SIM908 Use '{dictname}.get({key})' instead of "
+        "'if {key} in {dictname}: {dictname}[{key}]'"
+    )
+    errors: List[Tuple[int, int, str]] = []
+    if not (
+        isinstance(node.test, ast.Compare)
+        and len(node.test.ops) == 1
+        and isinstance(node.test.ops[0], ast.In)
+        and len(node.body) == 1
+        and len(node.orelse) == 0
+    ):
+        return errors
+    # We might still be left with a check if a value is in a list or in
+    # the body the developer might remove the element from the list
+    # We need to have a look at the body
+    if not (
+        isinstance(node.body[0], ast.Assign)
+        and isinstance(node.body[0].value, ast.Subscript)
+        and len(node.body[0].targets) == 1
+        and isinstance(node.body[0].targets[0], ast.Name)
+    ):
+        return errors
+    key = to_source(node.test.left)
+    dictname = to_source(node.test.comparators[0])
+    errors.append(
+        (
+            node.lineno,
+            node.col_offset,
+            RULE.format(key=key, dictname=dictname),
+        )
+    )
+    return errors
+
+
 def get_sim401(node: ast.If) -> List[Tuple[int, int, str]]:
     """
     Get all calls that should use default values for dictionary access.

tests/test_900_rules.py

@@ -181,3 +181,15 @@ def test_sim907():
     assert results == {
         "1:11 SIM907 Use 'Optional[int]' instead of 'Union[int, None]'"
     }
+
+
+def test_sim908():
+    results = _results(
+        """name = "some_default"
+if "some_key" in some_dict:
+    name = some_dict["some_key"]"""
+    )
+    assert results == {
+        "2:0 SIM908 Use 'some_dict.get(\"some_key\")' instead of "
+        '\'if "some_key" in some_dict: some_dict["some_key"]\''
+    }