Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add UseAssertIn lint rule to Fixit #159

Merged
merged 2 commits into from
Dec 8, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 171 additions & 0 deletions fixit/rules/use_assert_in.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# This source code is licensed under the MIT license found in the
# LICENSE file in the root directory of this source tree.

import libcst as cst
import libcst.matchers as m
from libcst.helpers import ensure_type

from fixit import CstLintRule, InvalidTestCase as Invalid, ValidTestCase as Valid


class UseAssertInRule(CstLintRule):
"""
Discourages use of ``assertTrue(x in y)`` and ``assertFalse(x in y)``
as it is deprecated (https://docs.python.org/3.8/library/unittest.html#deprecated-aliases).
Use ``assertIn(x, y)`` and ``assertNotIn(x, y)``) instead.
"""

MESSAGE: str = (
"Use assertIn/assertNotIn instead of assertTrue/assertFalse for inclusion check.\n"
+ "See https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertIn)"
)

VALID = [
Valid("self.assertIn(a, b)"),
Valid("self.assertIn(f(), b)"),
Valid("self.assertIn(f(x), b)"),
Valid("self.assertIn(f(g(x)), b)"),
Valid("self.assertNotIn(a, b)"),
Valid("self.assertNotIn(f(), b)"),
Valid("self.assertNotIn(f(x), b)"),
Valid("self.assertNotIn(f(g(x)), b)"),
]

INVALID = [
Invalid(
"self.assertTrue(a in b)",
expected_replacement="self.assertIn(a, b)",
),
Invalid(
"self.assertTrue(f() in b)",
expected_replacement="self.assertIn(f(), b)",
),
Invalid(
"self.assertTrue(f(x) in b)",
expected_replacement="self.assertIn(f(x), b)",
),
Invalid(
"self.assertTrue(f(g(x)) in b)",
expected_replacement="self.assertIn(f(g(x)), b)",
),
Invalid(
"self.assertTrue(a not in b)",
expected_replacement="self.assertNotIn(a, b)",
),
Invalid(
"self.assertTrue(not a in b)",
expected_replacement="self.assertNotIn(a, b)",
),
Invalid(
"self.assertFalse(a in b)",
expected_replacement="self.assertNotIn(a, b)",
),
]

def visit_Call(self, node: cst.Call) -> None:
# Todo: Make use of single extract instead of having several
# if else statemenets to make the code more robust and readable.
if m.matches(
node,
m.Call(
func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")),
Copy link
Contributor

@thatch thatch Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is based on some pre-existing code, but I think you can do this more effectively with a single extract; save whether you see the couple of places "not" could be, whether the method is assertTrue or assertFalse, and construct once with the xor of those 3 without a page of if statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense.
this was taken from the internal rule though. i'll leave it for now so there's consistency between both places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, use a single extract can be more effective and that can leave as future work. @dkteo can you add a # TODO: comment to the code as a note?
Also, once this is merged, we'll want to remove the internal copy to avoid duplication since this will be released to internal dev environment.

args=[
m.Arg(
m.Comparison(comparisons=[m.ComparisonTarget(operator=m.In())])
)
],
),
):
# self.assertTrue(a in b) -> self.assertIn(a, b)
new_call = node.with_changes(
func=cst.Attribute(value=cst.Name("self"), attr=cst.Name("assertIn")),
args=[
cst.Arg(ensure_type(node.args[0].value, cst.Comparison).left),
cst.Arg(
ensure_type(node.args[0].value, cst.Comparison)
.comparisons[0]
.comparator
),
],
)
self.report(node, replacement=new_call)
else:
# ... -> self.assertNotIn(a, b)
matched, arg1, arg2 = False, None, None
if m.matches(
node,
m.Call(
func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")),
args=[
m.Arg(
m.UnaryOperation(
operator=m.Not(),
expression=m.Comparison(
comparisons=[m.ComparisonTarget(operator=m.In())]
),
)
)
],
),
):
# self.assertTrue(not a in b) -> self.assertNotIn(a, b)
matched = True
arg1 = cst.Arg(
ensure_type(
ensure_type(node.args[0].value, cst.UnaryOperation).expression,
cst.Comparison,
).left
)
arg2 = cst.Arg(
ensure_type(
ensure_type(node.args[0].value, cst.UnaryOperation).expression,
cst.Comparison,
)
.comparisons[0]
.comparator
)
elif m.matches(
node,
m.Call(
func=m.Attribute(value=m.Name("self"), attr=m.Name("assertTrue")),
args=[
m.Arg(m.Comparison(comparisons=[m.ComparisonTarget(m.NotIn())]))
],
),
):
# self.assertTrue(a not in b) -> self.assertNotIn(a, b)
matched = True
arg1 = cst.Arg(ensure_type(node.args[0].value, cst.Comparison).left)
arg2 = cst.Arg(
ensure_type(node.args[0].value, cst.Comparison)
.comparisons[0]
.comparator
)
elif m.matches(
node,
m.Call(
func=m.Attribute(value=m.Name("self"), attr=m.Name("assertFalse")),
args=[
m.Arg(m.Comparison(comparisons=[m.ComparisonTarget(m.In())]))
],
),
):
# self.assertFalse(a in b) -> self.assertNotIn(a, b)
matched = True
arg1 = cst.Arg(ensure_type(node.args[0].value, cst.Comparison).left)
arg2 = cst.Arg(
ensure_type(node.args[0].value, cst.Comparison)
.comparisons[0]
.comparator
)

if matched:
new_call = node.with_changes(
func=cst.Attribute(
value=cst.Name("self"), attr=cst.Name("assertNotIn")
),
args=[arg1, arg2],
)
self.report(node, replacement=new_call)