From 94cc9891106e5c32eebbce7d26ccf27778690d90 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 12 May 2021 14:50:53 +0800 Subject: [PATCH 01/26] Implemented new check ``consider-using-str-partition`` --- ChangeLog | 5 ++ doc/whatsnew/2.9.rst | 3 + .../refactoring/recommendation_checker.py | 72 +++++++++++++++++++ .../consider/consider_using_str_partition.py | 68 ++++++++++++++++++ .../consider/consider_using_str_partition.txt | 21 ++++++ 5 files changed, 169 insertions(+) create mode 100644 tests/functional/c/consider/consider_using_str_partition.py create mode 100644 tests/functional/c/consider/consider_using_str_partition.txt diff --git a/ChangeLog b/ChangeLog index b4aa2906e8..8958edc98e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -43,6 +43,11 @@ modules are added. * Fix incompatibility with Python 3.6.0 caused by ``typing.Counter`` and ``typing.NoReturn`` usage Closes #4412 + +* New checker ``consider-using-str-partition``. Emitted when only accessing the first or last + element of a str.split(). + + Closes #4440 What's New in Pylint 2.8.2? diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 0a353d68ef..288ceb355e 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -15,6 +15,9 @@ New checkers * ``consider-using-dict-items``: Emitted when iterating over dictionary keys and then indexing the same dictionary with the key within loop body. +* ``consider-using-str-partition``: Emitted when accessing only the first or last element + of a str.split(). + Other Changes ============= diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 6efc4190c4..5540986f2d 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -35,6 +35,14 @@ class RecommendationChecker(checkers.BaseChecker): "Both the key and value can be accessed by iterating using the .items() " "method of the dictionary instead.", ), + "C0207": ( + "Consider using str.partition()", + "consider-using-str-partition", + "Emitted when accessing only the first or last element of a str.split(sep). " + "The first and last element can be accessed by using str.partition(sep)[0] " + "or str.rpartition(sep)[-1] instead, which is less computationally " + "expensive.", + ), } @staticmethod @@ -210,3 +218,67 @@ def visit_comprehension(self, node: astroid.Comprehension) -> None: self.add_message("consider-using-dict-items", node=node) return + + @utils.check_messages("consider-using-str-partition") + def visit_subscript(self, node: astroid.Subscript) -> None: + """Add message when accessing first or last elements of a str.split().""" + assignment = None + subscripted_object = node.value + if isinstance(subscripted_object, astroid.Name): + # Check assignment of this subscripted object + assignment_lookup_results = node.value.lookup(node.value.name) + if ( + len(assignment_lookup_results) == 0 + or len(assignment_lookup_results[-1]) == 0 + ): + return + assign_name = assignment_lookup_results[-1][-1] + if not isinstance(assign_name, astroid.AssignName): + return + assignment = assign_name.parent + if not isinstance(assignment, astroid.Assign): + return + # Set subscripted_object to the assignment RHS + subscripted_object = assignment.value + + if isinstance(subscripted_object, astroid.Call): + # Check if call is .split() or .rsplit() + if isinstance( + subscripted_object.func, astroid.Attribute + ) and subscripted_object.func.attrname in ["split", "rsplit"]: + inferred = utils.safe_infer(subscripted_object.func) + if not isinstance(inferred, astroid.BoundMethod): + return + + # Check if subscript is 1 or -1 + value = node.slice + if isinstance(value, astroid.Index): + value = value.value + + if isinstance(value, (astroid.UnaryOp, astroid.Const)): + const = utils.safe_infer(value) + const = cast(astroid.Const, const) + if const.value in [-1, 0]: + self.add_message("consider-using-str-partition", node=node) + + # Check if subscript is len(split) - 1 + if ( + isinstance(value, astroid.BinOp) + and value.left.func.name == "len" + and value.op == "-" + and value.right.value == 1 + and assignment is not None + ): + # Ensure that the len() is from builtins, not user re-defined + inferred_fn = utils.safe_infer(value.left.func) + if not ( + isinstance(inferred_fn, astroid.FunctionDef) + and isinstance(inferred_fn.parent, astroid.Module) + and inferred_fn.parent.name == "builtins" + ): + return + + # Further check for argument within len(), and compare that to object being split + arg_within_len = value.left.args[0].name + if arg_within_len == node.value.name: + self.add_message("consider-using-str-partition", node=node) diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py new file mode 100644 index 0000000000..935f1a3938 --- /dev/null +++ b/tests/functional/c/consider/consider_using_str_partition.py @@ -0,0 +1,68 @@ +"""Emit a message for iteration through dict keys and subscripting dict with key.""" +# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin +from collections.abc import Iterable +from typing import Any + +SEQ = '1,2,3' +get_first = SEQ.split(',')[0] # [consider-using-str-partition] +get_last = SEQ.split(',')[-1] # [consider-using-str-partition] +get_first = SEQ.rsplit(',')[0] # [consider-using-str-partition] +get_last = SEQ.rsplit(',')[-1] # [consider-using-str-partition] + +get_mid = SEQ.split(',')[1] # This is okay +get_mid = SEQ.split(',')[-2] # This is okay + +# Test with storing splits as an object +split1 = SEQ.split(',') +get_first = split1[0] # [consider-using-str-partition] +get_last = split1[-1] # [consider-using-str-partition] +get_last = split1[len(split1)-1] # [consider-using-str-partition] + +split2 = SEQ.rsplit(',') +get_first = split2[0] # [consider-using-str-partition] +get_last = split2[-1] # [consider-using-str-partition] +get_last = split2[len(split2)-1] # [consider-using-str-partition] + +print(split1[0], split1[-1]) # [consider-using-str-partition, consider-using-str-partition] + +# Test when running len on another iterable +some_list = [] +get_last = split1[len(split2)-1] # Should not throw an error + +# Tests on class attributes +class Foo(): + class_str = '1,2,3' + + def __init__(self): + self.my_str = '1,2,3' + + def get_string(self) -> str: + return self.my_str + +# Class attributes +get_first = Foo.class_str.split(',')[0] # [consider-using-str-partition] +get_last = Foo.class_str.split(',')[-1] # [consider-using-str-partition] +get_first = Foo.class_str.rsplit(',')[0] # [consider-using-str-partition] +get_last = Foo.class_str.rsplit(',')[-1] # [consider-using-str-partition] + +get_mid = Foo.class_str.split(',')[1] +get_mid = Foo.class_str.split(',')[-2] + +split2 = Foo.class_str.split(',') +get_first = split2[0] # [consider-using-str-partition] +get_last = split2[-1] # [consider-using-str-partition] +get_last = split2[len(split2)-1] # [consider-using-str-partition] + +# Test with accessors +bar = Foo() +get_first = bar.get_string().split(',')[0] # [consider-using-str-partition] +get_last = bar.get_string().split(',')[-1] # [consider-using-str-partition] + +get_mid = bar.get_string().split(',')[1] +get_mid = bar.get_string().split(',')[-2] + +# Test with user-defined len function +def len(x: Iterable[Any]) -> str: + return f"Hello, world! {x[2]}" + +get_last = split2[len(split2)-1] # This won't throw the warning as the len function has been redefined diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt new file mode 100644 index 0000000000..ebc34c9f35 --- /dev/null +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -0,0 +1,21 @@ +consider-using-str-partition:7:12::Consider using str.partition() +consider-using-str-partition:8:11::Consider using str.partition() +consider-using-str-partition:9:12::Consider using str.partition() +consider-using-str-partition:10:11::Consider using str.partition() +consider-using-str-partition:17:12::Consider using str.partition() +consider-using-str-partition:18:11::Consider using str.partition() +consider-using-str-partition:19:11::Consider using str.partition() +consider-using-str-partition:22:12::Consider using str.partition() +consider-using-str-partition:23:11::Consider using str.partition() +consider-using-str-partition:24:11::Consider using str.partition() +consider-using-str-partition:26:6::Consider using str.partition() +consider-using-str-partition:26:17::Consider using str.partition() +consider-using-str-partition:43:12::Consider using str.partition() +consider-using-str-partition:44:11::Consider using str.partition() +consider-using-str-partition:45:12::Consider using str.partition() +consider-using-str-partition:46:11::Consider using str.partition() +consider-using-str-partition:52:12::Consider using str.partition() +consider-using-str-partition:53:11::Consider using str.partition() +consider-using-str-partition:54:11::Consider using str.partition() +consider-using-str-partition:58:12::Consider using str.partition() +consider-using-str-partition:59:11::Consider using str.partition() From 3173a45f5e84dccce4a51b89e55fa19a21d546d9 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 12 May 2021 15:13:38 +0800 Subject: [PATCH 02/26] Changed source of lint_module_test due to new checkers breaking --- pylint/testutils/lint_module_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pylint/testutils/lint_module_test.py b/pylint/testutils/lint_module_test.py index e3580946d2..c970be92ab 100644 --- a/pylint/testutils/lint_module_test.py +++ b/pylint/testutils/lint_module_test.py @@ -52,7 +52,9 @@ def __init__(self, test_file: FunctionalTestFile, config: Optional[Config] = Non def setUp(self): if self._should_be_skipped_due_to_version(): - pytest.skip("Test cannot run with Python %s." % sys.version.split(" ")[0]) + pytest.skip( + "Test cannot run with Python %s." % sys.version.partition(" ")[0] + ) missing = [] for requirement in self._test_file.options["requires"]: try: From 3b36d2a96e97adbde9cee3dc4c6d842f42c0af28 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 12 May 2021 16:13:03 +0800 Subject: [PATCH 03/26] Improved help text for ``consider-using-str-partition`` --- .../refactoring/recommendation_checker.py | 29 ++++++++++--- .../consider/consider_using_str_partition.txt | 42 +++++++++---------- 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 5540986f2d..af7993dc4e 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -1,5 +1,6 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html # For details: https://github.com/PyCQA/pylint/blob/master/LICENSE +import re from typing import cast import astroid @@ -36,7 +37,7 @@ class RecommendationChecker(checkers.BaseChecker): "method of the dictionary instead.", ), "C0207": ( - "Consider using str.partition()", + "Consider using %s[%d] instead", "consider-using-str-partition", "Emitted when accessing only the first or last element of a str.split(sep). " "The first and last element can be accessed by using str.partition(sep)[0] " @@ -258,9 +259,21 @@ def visit_subscript(self, node: astroid.Subscript) -> None: if isinstance(value, (astroid.UnaryOp, astroid.Const)): const = utils.safe_infer(value) const = cast(astroid.Const, const) - if const.value in [-1, 0]: - self.add_message("consider-using-str-partition", node=node) - + p = re.compile(r"r*split") + if const.value == -1: + help_text = p.sub("rpartition", subscripted_object.as_string()) + self.add_message( + "consider-using-str-partition", + node=node, + args=(help_text, -1), + ) + elif const.value == 0: + help_text = p.sub("partition", subscripted_object.as_string()) + self.add_message( + "consider-using-str-partition", + node=node, + args=(help_text, 0), + ) # Check if subscript is len(split) - 1 if ( isinstance(value, astroid.BinOp) @@ -281,4 +294,10 @@ def visit_subscript(self, node: astroid.Subscript) -> None: # Further check for argument within len(), and compare that to object being split arg_within_len = value.left.args[0].name if arg_within_len == node.value.name: - self.add_message("consider-using-str-partition", node=node) + p = re.compile(r"r*split") + help_text = p.sub("rpartition", subscripted_object.as_string()) + self.add_message( + "consider-using-str-partition", + node=node, + args=(help_text, -1), + ) diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index ebc34c9f35..a6e3f4e39a 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -1,21 +1,21 @@ -consider-using-str-partition:7:12::Consider using str.partition() -consider-using-str-partition:8:11::Consider using str.partition() -consider-using-str-partition:9:12::Consider using str.partition() -consider-using-str-partition:10:11::Consider using str.partition() -consider-using-str-partition:17:12::Consider using str.partition() -consider-using-str-partition:18:11::Consider using str.partition() -consider-using-str-partition:19:11::Consider using str.partition() -consider-using-str-partition:22:12::Consider using str.partition() -consider-using-str-partition:23:11::Consider using str.partition() -consider-using-str-partition:24:11::Consider using str.partition() -consider-using-str-partition:26:6::Consider using str.partition() -consider-using-str-partition:26:17::Consider using str.partition() -consider-using-str-partition:43:12::Consider using str.partition() -consider-using-str-partition:44:11::Consider using str.partition() -consider-using-str-partition:45:12::Consider using str.partition() -consider-using-str-partition:46:11::Consider using str.partition() -consider-using-str-partition:52:12::Consider using str.partition() -consider-using-str-partition:53:11::Consider using str.partition() -consider-using-str-partition:54:11::Consider using str.partition() -consider-using-str-partition:58:12::Consider using str.partition() -consider-using-str-partition:59:11::Consider using str.partition() +consider-using-str-partition:7:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:8:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:9:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:10:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:17:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:18:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:19:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:22:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:23:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:24:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:26:6::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:26:17::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:43:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:44:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:45:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:46:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:52:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:53:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:54:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:58:12::Consider using bar.get_string().partition(',')[0] instead +consider-using-str-partition:59:11::Consider using bar.get_string().rpartition(',')[-1] instead From cc61bf5b7e9b84b72605d2618faca03fd5e4b91a Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 12 May 2021 17:30:08 +0800 Subject: [PATCH 04/26] Added additional tests --- .../functional/c/consider/consider_using_str_partition.py | 7 +++++++ .../functional/c/consider/consider_using_str_partition.txt | 2 ++ 2 files changed, 9 insertions(+) diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py index 935f1a3938..226b5dd8ac 100644 --- a/tests/functional/c/consider/consider_using_str_partition.py +++ b/tests/functional/c/consider/consider_using_str_partition.py @@ -61,6 +61,13 @@ def get_string(self) -> str: get_mid = bar.get_string().split(',')[1] get_mid = bar.get_string().split(',')[-2] +# Test with iterating over strings +list_of_strs = ["a", "b", "c", "d", "e", "f"] +for s in list_of_strs: + print(s.split(" ")[0]) # [consider-using-str-partition] + print(s.split(" ")[-1]) # [consider-using-str-partition] + print(s.split(" ")[-2]) + # Test with user-defined len function def len(x: Iterable[Any]) -> str: return f"Hello, world! {x[2]}" diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index a6e3f4e39a..1899d22294 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -19,3 +19,5 @@ consider-using-str-partition:53:11::Consider using Foo.class_str.rpartition(',') consider-using-str-partition:54:11::Consider using Foo.class_str.rpartition(',')[-1] instead consider-using-str-partition:58:12::Consider using bar.get_string().partition(',')[0] instead consider-using-str-partition:59:11::Consider using bar.get_string().rpartition(',')[-1] instead +consider-using-str-partition:67:10::Consider using s.partition(' ')[0] instead +consider-using-str-partition:68:10::Consider using s.rpartition(' ')[-1] instead From eeb45d65667289c3614a07da0594f5e5ea968977 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 12 May 2021 22:44:26 +0800 Subject: [PATCH 05/26] Improved on help text logic and accompanying tests --- .../refactoring/recommendation_checker.py | 26 ++++++++-- .../consider/consider_using_str_partition.py | 12 +++++ .../consider/consider_using_str_partition.txt | 52 +++++++++++-------- 3 files changed, 62 insertions(+), 28 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index af7993dc4e..a012cb5216 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -259,16 +259,26 @@ def visit_subscript(self, node: astroid.Subscript) -> None: if isinstance(value, (astroid.UnaryOp, astroid.Const)): const = utils.safe_infer(value) const = cast(astroid.Const, const) - p = re.compile(r"r*split") + p = re.compile( + r"tilpsr*\." + ) # Match and replace in reverse to ensure last occurance replaced if const.value == -1: - help_text = p.sub("rpartition", subscripted_object.as_string()) + help_text = p.sub( + ".rpartition"[::-1], + subscripted_object.as_string()[::-1], + count=1, + )[::-1] self.add_message( "consider-using-str-partition", node=node, args=(help_text, -1), ) elif const.value == 0: - help_text = p.sub("partition", subscripted_object.as_string()) + help_text = p.sub( + ".partition"[::-1], + subscripted_object.as_string()[::-1], + count=1, + )[::-1] self.add_message( "consider-using-str-partition", node=node, @@ -294,8 +304,14 @@ def visit_subscript(self, node: astroid.Subscript) -> None: # Further check for argument within len(), and compare that to object being split arg_within_len = value.left.args[0].name if arg_within_len == node.value.name: - p = re.compile(r"r*split") - help_text = p.sub("rpartition", subscripted_object.as_string()) + p = re.compile( + r"tilpsr*\." + ) # Match and replace in reverse to ensure last occurance replaced + help_text = p.sub( + ".rpartition"[::-1], + subscripted_object.as_string()[::-1], + count=1, + )[::-1] self.add_message( "consider-using-str-partition", node=node, diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py index 226b5dd8ac..541cff6443 100644 --- a/tests/functional/c/consider/consider_using_str_partition.py +++ b/tests/functional/c/consider/consider_using_str_partition.py @@ -3,6 +3,9 @@ from collections.abc import Iterable from typing import Any +get_first = '1,2,3'.split(',')[0] # [consider-using-str-partition] +get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-str-partition] + SEQ = '1,2,3' get_first = SEQ.split(',')[0] # [consider-using-str-partition] get_last = SEQ.split(',')[-1] # [consider-using-str-partition] @@ -68,6 +71,15 @@ def get_string(self) -> str: print(s.split(" ")[-1]) # [consider-using-str-partition] print(s.split(" ")[-2]) +# Test warning messages (matching and replacing .split / .rsplit) +class Bar(): + split = '1,2,3' + +print(Bar.split.split(",")[0]) # [consider-using-str-partition] (Error message should show Bar.split.partition) +print(Bar.split.split(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) +print(Bar.split.rsplit(",")[0]) # [consider-using-str-partition] (Error message should show Bar.split.partition) +print(Bar.split.rsplit(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) + # Test with user-defined len function def len(x: Iterable[Any]) -> str: return f"Hello, world! {x[2]}" diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index 1899d22294..25e400b294 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -1,23 +1,29 @@ -consider-using-str-partition:7:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:8:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:9:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:10:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:17:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:18:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:19:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:22:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:23:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:24:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:26:6::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:26:17::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:43:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:44:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:45:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:46:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:52:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:53:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:54:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:58:12::Consider using bar.get_string().partition(',')[0] instead -consider-using-str-partition:59:11::Consider using bar.get_string().rpartition(',')[-1] instead -consider-using-str-partition:67:10::Consider using s.partition(' ')[0] instead -consider-using-str-partition:68:10::Consider using s.rpartition(' ')[-1] instead +consider-using-str-partition:6:12::Consider using '1,2,3'.partition(',')[0] instead +consider-using-str-partition:7:11::"Consider using '1,2,3'[::-1].partition(',')[0] instead" +consider-using-str-partition:10:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:11:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:12:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:13:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:20:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:21:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:22:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:25:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:26:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:27:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:29:6::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:29:17::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:46:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:47:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:48:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:49:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:55:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:56:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:57:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:61:12::Consider using bar.get_string().partition(',')[0] instead +consider-using-str-partition:62:11::Consider using bar.get_string().rpartition(',')[-1] instead +consider-using-str-partition:70:10::Consider using s.partition(' ')[0] instead +consider-using-str-partition:71:10::Consider using s.rpartition(' ')[-1] instead +consider-using-str-partition:78:6::Consider using Bar.split.partition(',')[0] instead +consider-using-str-partition:79:6::Consider using Bar.split.rpartition(',')[-1] instead +consider-using-str-partition:80:6::Consider using Bar.split.partition(',')[0] instead +consider-using-str-partition:81:6::Consider using Bar.split.rpartition(',')[-1] instead From 943bd58bd7c3a34630268ff3fa6eaef87f8b0f51 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Thu, 13 May 2021 00:34:46 +0800 Subject: [PATCH 06/26] Update docs --- ChangeLog | 5 +++-- doc/whatsnew/2.9.rst | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8958edc98e..32fd1c5581 100644 --- a/ChangeLog +++ b/ChangeLog @@ -44,8 +44,9 @@ modules are added. Closes #4412 -* New checker ``consider-using-str-partition``. Emitted when only accessing the first or last - element of a str.split(). +* New checker ``consider-using-str-partition``. Emitted either when only accessing the first or last + element of a ``str.split()`` or ``str.rsplit()``, or when a ``str.split()`` or ``str.rsplit()`` is + used with a ``max_split`` argument value of 1. Closes #4440 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 288ceb355e..32894cc624 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -16,7 +16,8 @@ New checkers indexing the same dictionary with the key within loop body. * ``consider-using-str-partition``: Emitted when accessing only the first or last element - of a str.split(). + of a ``str.split()`` or ``str.rsplit()``, or when a ``str.split()`` or ``str.rsplit()`` is + used with a ``max_split`` argument value of 1. Other Changes ============= From 5963aac7a45fe2b37e9712cf9139b5c2802b64b3 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Thu, 13 May 2021 12:39:02 +0800 Subject: [PATCH 07/26] Reworked consider-using-str-partition --- .../refactoring/recommendation_checker.py | 236 ++++++++++-------- .../consider/consider_using_str_partition.py | 68 +++-- .../consider/consider_using_str_partition.txt | 54 ++-- 3 files changed, 201 insertions(+), 157 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index a012cb5216..dff0de925e 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -1,6 +1,5 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html # For details: https://github.com/PyCQA/pylint/blob/master/LICENSE -import re from typing import cast import astroid @@ -37,12 +36,14 @@ class RecommendationChecker(checkers.BaseChecker): "method of the dictionary instead.", ), "C0207": ( - "Consider using %s[%d] instead", + "Consider using %s instead", "consider-using-str-partition", "Emitted when accessing only the first or last element of a str.split(sep). " + "Or when a str.split(sep,maxsplit=1) is used. " "The first and last element can be accessed by using str.partition(sep)[0] " "or str.rpartition(sep)[-1] instead, which is less computationally " - "expensive.", + "expensive and works the same as str.split() or str.rsplit() with a maxsplit " + "of 1", ), } @@ -53,8 +54,14 @@ def _is_builtin(node, function): return False return utils.is_builtin_object(inferred) and inferred.name == function - @utils.check_messages("consider-iterating-dictionary") - def visit_call(self, node): + @utils.check_messages( + "consider-iterating-dictionary", "consider-using-str-partition" + ) + def visit_call(self, node: astroid.Call) -> None: + self._check_consider_iterating_dictionary(node) + self._check_consider_using_str_partition(node) + + def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: if not isinstance(node.func, astroid.Attribute): return if node.func.attrname != "keys": @@ -71,6 +78,127 @@ def visit_call(self, node): if isinstance(node.parent, (astroid.For, astroid.Comprehension)): self.add_message("consider-iterating-dictionary", node=node) + def _check_consider_using_str_partition(self, node: astroid.Call) -> None: + """Add message when accessing first or last elements of a str.split() or str.rsplit(), + or when split/rsplit with max_split=1 is used""" + + # Check if call is split() or rsplit() + if isinstance(node.func, astroid.Attribute) and node.func.attrname in ( + "split", + "rsplit", + ): + inferred_func = utils.safe_infer(node.func) + fn_name = node.func.attrname + node_name = node.as_string() + + if not isinstance(inferred_func, astroid.BoundMethod): + return + + try: + seperator = utils.get_argument_from_call(node, 0, "sep").value + except utils.NoSuchArgumentError: + return + # Check if maxsplit is set, and if it's == 1 (this can be replaced by partition) + try: + maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value + if maxsplit == 1: + self.add_message( + "consider-using-str-partition", + node=node, + args=( + f"{node.func.expr.as_string()}.{node.func.attrname.replace('split','partition')}('{seperator}')" + ), + ) + return + + except utils.NoSuchArgumentError: + pass + + # Check if it's immediately subscripted + if isinstance(node.parent, astroid.Subscript): + subscript_node = node.parent + # Check if subscripted with -1/0 + if not isinstance( + subscript_node.slice, (astroid.Const, astroid.UnaryOp) + ): + return + subscript_value = utils.safe_infer(subscript_node.slice) + subscript_value = cast(astroid.Const, subscript_value) + subscript_value = subscript_value.value + if subscript_value in (-1, 0): + new_fn = "rpartition" if subscript_value == -1 else "partition" + new_fn = new_fn[::-1] + node_name = node_name[::-1] + fn_name = fn_name[::-1] + new_name = node_name.replace(fn_name, new_fn, 1)[::-1] + self.add_message( + "consider-using-str-partition", node=node, args=(new_name,) + ) + return + # Check where name it's assigned to, then check all usage of name + assign_target = None + if isinstance(node.parent, astroid.Tuple): + assign_node = node.parent.parent + if not isinstance(assign_node, astroid.Assign): + return + idx = node.parent.elts.index(node) + if not isinstance(assign_node.targets[0], astroid.Tuple): + return + assign_target = assign_node.targets[0].elts[idx] + elif isinstance(node.parent, astroid.Assign): + assign_target = node.parent.targets[0] + + if assign_target is None or not isinstance( + assign_target, astroid.AssignName + ): + return + + # Go to outer-most scope (module), then search the child for usage + module_node = node + while not isinstance(module_node, astroid.Module): + module_node = module_node.parent + + subscript_usage = set() + for child in module_node.body: + for search_node in child.nodes_of_class(astroid.Name): + search_node = cast(astroid.Name, search_node) + + last_definition = search_node.lookup(search_node.name)[1][-1] + if last_definition is not assign_target: + continue + if not isinstance(search_node.parent, astroid.Subscript): + continue + subscript_node = search_node.parent + if not isinstance( + subscript_node.slice, (astroid.Const, astroid.UnaryOp) + ): + return + subscript_value = utils.safe_infer(subscript_node.slice) + subscript_value = cast(astroid.Const, subscript_value) + subscript_value = subscript_value.value + if subscript_value not in (-1, 0): + return + subscript_usage.add(subscript_value) + if not subscript_usage: # Not used + return + # Construct help text + help_text = "" + node_name = node_name[::-1] + fn_name = fn_name[::-1] + if 0 in subscript_usage: + new_fn = "partition"[::-1] + new_name = node_name.replace(fn_name, new_fn, 1)[::-1] + help_text += f"{new_name} to extract the first element of a .split()" + + if -1 in subscript_usage: + new_fn = "rpartition"[::-1] + new_name = node_name.replace(fn_name, new_fn, 1)[::-1] + help_text += " and " if help_text != "" else "" + help_text += f"{new_name} to extract the last element of a .split()" + self.add_message( + "consider-using-str-partition", node=node, args=(help_text,) + ) + @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") def visit_for(self, node: astroid.For) -> None: self._check_consider_using_enumerate(node) @@ -219,101 +347,3 @@ def visit_comprehension(self, node: astroid.Comprehension) -> None: self.add_message("consider-using-dict-items", node=node) return - - @utils.check_messages("consider-using-str-partition") - def visit_subscript(self, node: astroid.Subscript) -> None: - """Add message when accessing first or last elements of a str.split().""" - assignment = None - subscripted_object = node.value - if isinstance(subscripted_object, astroid.Name): - # Check assignment of this subscripted object - assignment_lookup_results = node.value.lookup(node.value.name) - if ( - len(assignment_lookup_results) == 0 - or len(assignment_lookup_results[-1]) == 0 - ): - return - assign_name = assignment_lookup_results[-1][-1] - if not isinstance(assign_name, astroid.AssignName): - return - assignment = assign_name.parent - if not isinstance(assignment, astroid.Assign): - return - # Set subscripted_object to the assignment RHS - subscripted_object = assignment.value - - if isinstance(subscripted_object, astroid.Call): - # Check if call is .split() or .rsplit() - if isinstance( - subscripted_object.func, astroid.Attribute - ) and subscripted_object.func.attrname in ["split", "rsplit"]: - inferred = utils.safe_infer(subscripted_object.func) - if not isinstance(inferred, astroid.BoundMethod): - return - - # Check if subscript is 1 or -1 - value = node.slice - if isinstance(value, astroid.Index): - value = value.value - - if isinstance(value, (astroid.UnaryOp, astroid.Const)): - const = utils.safe_infer(value) - const = cast(astroid.Const, const) - p = re.compile( - r"tilpsr*\." - ) # Match and replace in reverse to ensure last occurance replaced - if const.value == -1: - help_text = p.sub( - ".rpartition"[::-1], - subscripted_object.as_string()[::-1], - count=1, - )[::-1] - self.add_message( - "consider-using-str-partition", - node=node, - args=(help_text, -1), - ) - elif const.value == 0: - help_text = p.sub( - ".partition"[::-1], - subscripted_object.as_string()[::-1], - count=1, - )[::-1] - self.add_message( - "consider-using-str-partition", - node=node, - args=(help_text, 0), - ) - # Check if subscript is len(split) - 1 - if ( - isinstance(value, astroid.BinOp) - and value.left.func.name == "len" - and value.op == "-" - and value.right.value == 1 - and assignment is not None - ): - # Ensure that the len() is from builtins, not user re-defined - inferred_fn = utils.safe_infer(value.left.func) - if not ( - isinstance(inferred_fn, astroid.FunctionDef) - and isinstance(inferred_fn.parent, astroid.Module) - and inferred_fn.parent.name == "builtins" - ): - return - - # Further check for argument within len(), and compare that to object being split - arg_within_len = value.left.args[0].name - if arg_within_len == node.value.name: - p = re.compile( - r"tilpsr*\." - ) # Match and replace in reverse to ensure last occurance replaced - help_text = p.sub( - ".rpartition"[::-1], - subscripted_object.as_string()[::-1], - count=1, - )[::-1] - self.add_message( - "consider-using-str-partition", - node=node, - args=(help_text, -1), - ) diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py index 541cff6443..b4cce175ad 100644 --- a/tests/functional/c/consider/consider_using_str_partition.py +++ b/tests/functional/c/consider/consider_using_str_partition.py @@ -1,8 +1,19 @@ """Emit a message for iteration through dict keys and subscripting dict with key.""" # pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin -from collections.abc import Iterable -from typing import Any + +# Test varying maxsplit argument +bad_split = '1,2,3'.split(sep=',',maxsplit=1) # [consider-using-str-partition] +bad_split = '1,2,3'[::-1].split(',',1) # [consider-using-str-partition] + +good_split = '1,2,3'[::-1].split(',',2) +good_split = '1,2,3'[::-1].split(',') + +good_split = '1,2,3'[::-1].split(',',2)[0] #This is fine, we ignore cases where maxsplit > 1 +good_split = '1,2,3'[::-1].split(',',2)[-1] #This is fine, we ignore cases where maxsplit > 1 + + +# Test subscripting .split() get_first = '1,2,3'.split(',')[0] # [consider-using-str-partition] get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-str-partition] @@ -15,27 +26,38 @@ get_mid = SEQ.split(',')[1] # This is okay get_mid = SEQ.split(',')[-2] # This is okay + # Test with storing splits as an object -split1 = SEQ.split(',') -get_first = split1[0] # [consider-using-str-partition] -get_last = split1[-1] # [consider-using-str-partition] -get_last = split1[len(split1)-1] # [consider-using-str-partition] +split1 = SEQ.split(',') # [consider-using-str-partition] +get_first = split1[0] +get_last = split1[-1] + +split2 = SEQ.rsplit(',') # [consider-using-str-partition] +get_first = split2[0] +get_last = split2[-1] -split2 = SEQ.rsplit(',') -get_first = split2[0] # [consider-using-str-partition] -get_last = split2[-1] # [consider-using-str-partition] -get_last = split2[len(split2)-1] # [consider-using-str-partition] +split3 = SEQ.rsplit(',') # This is fine, split3 indexed with [1] +get_first = split3[0] +get_middle = split3[1] +get_last = split3[-1] -print(split1[0], split1[-1]) # [consider-using-str-partition, consider-using-str-partition] +split1, split2 = SEQ.split(','), SEQ.rsplit(',') # [consider-using-str-partition, consider-using-str-partition] +get_first = split1[0] +get_last = split1[-1] +get_first = split2[0] +get_last = split2[-1] + +split1, split2 = SEQ.split(','), SEQ.rsplit(',') # This is fine, both splits are utilized +get_first = split1[0] +get_last = split1[-1] +get_first = split2[0] +get_last = split2[-1] +split1[1] = split2[1] -# Test when running len on another iterable -some_list = [] -get_last = split1[len(split2)-1] # Should not throw an error # Tests on class attributes class Foo(): class_str = '1,2,3' - def __init__(self): self.my_str = '1,2,3' @@ -51,10 +73,10 @@ def get_string(self) -> str: get_mid = Foo.class_str.split(',')[1] get_mid = Foo.class_str.split(',')[-2] -split2 = Foo.class_str.split(',') -get_first = split2[0] # [consider-using-str-partition] -get_last = split2[-1] # [consider-using-str-partition] -get_last = split2[len(split2)-1] # [consider-using-str-partition] +split2 = Foo.class_str.split(',') # [consider-using-str-partition] +get_first = split2[0] +get_last = split2[-1] + # Test with accessors bar = Foo() @@ -64,6 +86,7 @@ def get_string(self) -> str: get_mid = bar.get_string().split(',')[1] get_mid = bar.get_string().split(',')[-2] + # Test with iterating over strings list_of_strs = ["a", "b", "c", "d", "e", "f"] for s in list_of_strs: @@ -71,6 +94,7 @@ def get_string(self) -> str: print(s.split(" ")[-1]) # [consider-using-str-partition] print(s.split(" ")[-2]) + # Test warning messages (matching and replacing .split / .rsplit) class Bar(): split = '1,2,3' @@ -79,9 +103,3 @@ class Bar(): print(Bar.split.split(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) print(Bar.split.rsplit(",")[0]) # [consider-using-str-partition] (Error message should show Bar.split.partition) print(Bar.split.rsplit(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) - -# Test with user-defined len function -def len(x: Iterable[Any]) -> str: - return f"Hello, world! {x[2]}" - -get_last = split2[len(split2)-1] # This won't throw the warning as the len function has been redefined diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index 25e400b294..3d176b4ca7 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -1,29 +1,25 @@ -consider-using-str-partition:6:12::Consider using '1,2,3'.partition(',')[0] instead -consider-using-str-partition:7:11::"Consider using '1,2,3'[::-1].partition(',')[0] instead" -consider-using-str-partition:10:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:11:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:12:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:13:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:20:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:21:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:22:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:25:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:26:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:27:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:29:6::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:29:17::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:46:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:47:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:48:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:49:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:55:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:56:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:57:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:61:12::Consider using bar.get_string().partition(',')[0] instead -consider-using-str-partition:62:11::Consider using bar.get_string().rpartition(',')[-1] instead -consider-using-str-partition:70:10::Consider using s.partition(' ')[0] instead -consider-using-str-partition:71:10::Consider using s.rpartition(' ')[-1] instead -consider-using-str-partition:78:6::Consider using Bar.split.partition(',')[0] instead -consider-using-str-partition:79:6::Consider using Bar.split.rpartition(',')[-1] instead -consider-using-str-partition:80:6::Consider using Bar.split.partition(',')[0] instead -consider-using-str-partition:81:6::Consider using Bar.split.rpartition(',')[-1] instead +consider-using-str-partition:6:12::Consider using '1,2,3'.partition(',') instead +consider-using-str-partition:7:12::"Consider using '1,2,3'[::-1].partition(',') instead" +consider-using-str-partition:17:12::Consider using '1,2,3'.partition(',') instead +consider-using-str-partition:18:11::"Consider using '1,2,3'[::-1].partition(',') instead" +consider-using-str-partition:21:12::Consider using SEQ.partition(',') instead +consider-using-str-partition:22:11::Consider using SEQ.rpartition(',') instead +consider-using-str-partition:23:12::Consider using SEQ.partition(',') instead +consider-using-str-partition:24:11::Consider using SEQ.rpartition(',') instead +consider-using-str-partition:31:9::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead +consider-using-str-partition:35:9::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead +consider-using-str-partition:44:17::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead +consider-using-str-partition:44:33::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead +consider-using-str-partition:68:12::Consider using Foo.class_str.partition(',') instead +consider-using-str-partition:69:11::Consider using Foo.class_str.rpartition(',') instead +consider-using-str-partition:70:12::Consider using Foo.class_str.partition(',') instead +consider-using-str-partition:71:11::Consider using Foo.class_str.rpartition(',') instead +consider-using-str-partition:76:9::Consider using Foo.class_str.partition(',') to extract the first element of a .split() and Foo.class_str.rpartition(',') to extract the last element of a .split() instead +consider-using-str-partition:83:12::Consider using bar.get_string().partition(',') instead +consider-using-str-partition:84:11::Consider using bar.get_string().rpartition(',') instead +consider-using-str-partition:93:10::Consider using s.partition(' ') instead +consider-using-str-partition:94:10::Consider using s.rpartition(' ') instead +consider-using-str-partition:102:6::Consider using Bar.split.partition(',') instead +consider-using-str-partition:103:6::Consider using Bar.split.rpartition(',') instead +consider-using-str-partition:104:6::Consider using Bar.split.partition(',') instead +consider-using-str-partition:105:6::Consider using Bar.split.rpartition(',') instead From 64cba0c58a1839f55bb30f8e54ec07b7f6d67e79 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 13 May 2021 05:02:02 +0000 Subject: [PATCH 08/26] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 32fd1c5581..8ded59a2e8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -43,7 +43,7 @@ modules are added. * Fix incompatibility with Python 3.6.0 caused by ``typing.Counter`` and ``typing.NoReturn`` usage Closes #4412 - + * New checker ``consider-using-str-partition``. Emitted either when only accessing the first or last element of a ``str.split()`` or ``str.rsplit()``, or when a ``str.split()`` or ``str.rsplit()`` is used with a ``max_split`` argument value of 1. From 860784e42d38f9b1b4ebd8d35d6486cd77337d54 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Thu, 13 May 2021 13:12:00 +0800 Subject: [PATCH 09/26] Made checking subscript compatible with < py3.9 --- .../refactoring/recommendation_checker.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index dff0de925e..b1fd4e7c13 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -118,11 +118,12 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if isinstance(node.parent, astroid.Subscript): subscript_node = node.parent # Check if subscripted with -1/0 - if not isinstance( - subscript_node.slice, (astroid.Const, astroid.UnaryOp) - ): + value = subscript_node.slice + if isinstance(value, astroid.Index): + value = value.value + if not isinstance(value, (astroid.Const, astroid.UnaryOp)): return - subscript_value = utils.safe_infer(subscript_node.slice) + subscript_value = utils.safe_infer(value) subscript_value = cast(astroid.Const, subscript_value) subscript_value = subscript_value.value if subscript_value in (-1, 0): @@ -169,11 +170,14 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if not isinstance(search_node.parent, astroid.Subscript): continue subscript_node = search_node.parent - if not isinstance( - subscript_node.slice, (astroid.Const, astroid.UnaryOp) - ): + + value = subscript_node.slice + if isinstance(value, astroid.Index): + value = value.value + if not isinstance(value, (astroid.Const, astroid.UnaryOp)): return - subscript_value = utils.safe_infer(subscript_node.slice) + + subscript_value = utils.safe_infer(value) subscript_value = cast(astroid.Const, subscript_value) subscript_value = subscript_value.value if subscript_value not in (-1, 0): From 35854adc2e7335bca82617bb15fecdb0a195b615 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Thu, 13 May 2021 14:24:19 +0800 Subject: [PATCH 10/26] Refactored subscript checking to a utils function --- .../refactoring/recommendation_checker.py | 31 ++++++++----------- pylint/checkers/utils.py | 16 ++++++++++ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index b1fd4e7c13..1ca1714a04 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -118,14 +118,12 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if isinstance(node.parent, astroid.Subscript): subscript_node = node.parent # Check if subscripted with -1/0 - value = subscript_node.slice - if isinstance(value, astroid.Index): - value = value.value - if not isinstance(value, (astroid.Const, astroid.UnaryOp)): + try: + subscript_value = utils.get_subscript_const_value( + subscript_node + ).value + except ValueError: return - subscript_value = utils.safe_infer(value) - subscript_value = cast(astroid.Const, subscript_value) - subscript_value = subscript_value.value if subscript_value in (-1, 0): new_fn = "rpartition" if subscript_value == -1 else "partition" new_fn = new_fn[::-1] @@ -170,17 +168,14 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if not isinstance(search_node.parent, astroid.Subscript): continue subscript_node = search_node.parent - - value = subscript_node.slice - if isinstance(value, astroid.Index): - value = value.value - if not isinstance(value, (astroid.Const, astroid.UnaryOp)): - return - - subscript_value = utils.safe_infer(value) - subscript_value = cast(astroid.Const, subscript_value) - subscript_value = subscript_value.value - if subscript_value not in (-1, 0): + inferrable = True + try: + subscript_value = utils.get_subscript_const_value( + subscript_node + ).value + except ValueError: # Can't infer subscript value + inferrable = False + if not inferrable or subscript_value not in (-1, 0): return subscript_usage.add(subscript_value) if not subscript_usage: # Not used diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 8cf97d8074..79b01dac76 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -1522,3 +1522,19 @@ def get_iterating_dictionary_name( return node.iter.as_string() return None + + +def get_subscript_const_value(node: astroid.Subscript) -> astroid.Const: + """ + Returns the value (subscript.slice) of a Subscript node, + also supports python <3.9 windows where node.slice might be an Index + node + """ + value = node.slice + if isinstance(value, astroid.Index): + value = value.value + inferred = safe_infer(value) + if not isinstance(inferred, astroid.Const): + raise ValueError("Subscript.slice cannot be inferred as an astroid.Const") + + return inferred From e4b31b63ab0319fc183841a9ceb08ef59dd1102d Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Fri, 14 May 2021 21:09:28 +0800 Subject: [PATCH 11/26] Reworked and simplified ``consider-using-str-partition`` --- .../refactoring/recommendation_checker.py | 92 +++---------------- .../consider/consider_using_str_partition.py | 54 +++-------- .../consider/consider_using_str_partition.txt | 45 ++++----- 3 files changed, 50 insertions(+), 141 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 1ca1714a04..9fa493b747 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -93,26 +93,19 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if not isinstance(inferred_func, astroid.BoundMethod): return - try: seperator = utils.get_argument_from_call(node, 0, "sep").value except utils.NoSuchArgumentError: return - # Check if maxsplit is set, and if it's == 1 (this can be replaced by partition) + + # Check if maxsplit is set, and ignore checking if maxsplit is > 1 try: maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value - if maxsplit == 1: - self.add_message( - "consider-using-str-partition", - node=node, - args=( - f"{node.func.expr.as_string()}.{node.func.attrname.replace('split','partition')}('{seperator}')" - ), - ) - return + if maxsplit > 1: + return except utils.NoSuchArgumentError: - pass + maxsplit = 0 # Check if it's immediately subscripted if isinstance(node.parent, astroid.Subscript): @@ -124,79 +117,24 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: ).value except ValueError: return - if subscript_value in (-1, 0): + + if maxsplit == 1 and subscript_value == 1: + new_func = node.func.attrname.replace("split", "partition") + new_name = f"{node.as_string().rpartition('.')[0]}.{new_func}({seperator})[-1]" + self.add_message( + "consider-using-str-partition", node=node, args=(new_name,) + ) + + if maxsplit == 0 and subscript_value in (-1, 0): new_fn = "rpartition" if subscript_value == -1 else "partition" new_fn = new_fn[::-1] node_name = node_name[::-1] fn_name = fn_name[::-1] - new_name = node_name.replace(fn_name, new_fn, 1)[::-1] + new_name = f"{node_name.replace(fn_name, new_fn, 1)[::-1]}[{subscript_value}]" self.add_message( "consider-using-str-partition", node=node, args=(new_name,) ) return - # Check where name it's assigned to, then check all usage of name - assign_target = None - if isinstance(node.parent, astroid.Tuple): - assign_node = node.parent.parent - if not isinstance(assign_node, astroid.Assign): - return - idx = node.parent.elts.index(node) - if not isinstance(assign_node.targets[0], astroid.Tuple): - return - assign_target = assign_node.targets[0].elts[idx] - elif isinstance(node.parent, astroid.Assign): - assign_target = node.parent.targets[0] - - if assign_target is None or not isinstance( - assign_target, astroid.AssignName - ): - return - - # Go to outer-most scope (module), then search the child for usage - module_node = node - while not isinstance(module_node, astroid.Module): - module_node = module_node.parent - - subscript_usage = set() - for child in module_node.body: - for search_node in child.nodes_of_class(astroid.Name): - search_node = cast(astroid.Name, search_node) - - last_definition = search_node.lookup(search_node.name)[1][-1] - if last_definition is not assign_target: - continue - if not isinstance(search_node.parent, astroid.Subscript): - continue - subscript_node = search_node.parent - inferrable = True - try: - subscript_value = utils.get_subscript_const_value( - subscript_node - ).value - except ValueError: # Can't infer subscript value - inferrable = False - if not inferrable or subscript_value not in (-1, 0): - return - subscript_usage.add(subscript_value) - if not subscript_usage: # Not used - return - # Construct help text - help_text = "" - node_name = node_name[::-1] - fn_name = fn_name[::-1] - if 0 in subscript_usage: - new_fn = "partition"[::-1] - new_name = node_name.replace(fn_name, new_fn, 1)[::-1] - help_text += f"{new_name} to extract the first element of a .split()" - - if -1 in subscript_usage: - new_fn = "rpartition"[::-1] - new_name = node_name.replace(fn_name, new_fn, 1)[::-1] - help_text += " and " if help_text != "" else "" - help_text += f"{new_name} to extract the last element of a .split()" - self.add_message( - "consider-using-str-partition", node=node, args=(help_text,) - ) @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") def visit_for(self, node: astroid.For) -> None: diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py index b4cce175ad..f241f64412 100644 --- a/tests/functional/c/consider/consider_using_str_partition.py +++ b/tests/functional/c/consider/consider_using_str_partition.py @@ -1,18 +1,6 @@ """Emit a message for iteration through dict keys and subscripting dict with key.""" # pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin - -# Test varying maxsplit argument -bad_split = '1,2,3'.split(sep=',',maxsplit=1) # [consider-using-str-partition] -bad_split = '1,2,3'[::-1].split(',',1) # [consider-using-str-partition] - -good_split = '1,2,3'[::-1].split(',',2) -good_split = '1,2,3'[::-1].split(',') - -good_split = '1,2,3'[::-1].split(',',2)[0] #This is fine, we ignore cases where maxsplit > 1 -good_split = '1,2,3'[::-1].split(',',2)[-1] #This is fine, we ignore cases where maxsplit > 1 - - # Test subscripting .split() get_first = '1,2,3'.split(',')[0] # [consider-using-str-partition] get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-str-partition] @@ -27,32 +15,24 @@ get_mid = SEQ.split(',')[-2] # This is okay -# Test with storing splits as an object -split1 = SEQ.split(',') # [consider-using-str-partition] -get_first = split1[0] -get_last = split1[-1] - -split2 = SEQ.rsplit(',') # [consider-using-str-partition] -get_first = split2[0] -get_last = split2[-1] +# Test varying maxsplit argument +## str.split() tests +bad_split = '1,2,3'.split(sep=',', maxsplit=1)[1] # [consider-using-str-partition] -split3 = SEQ.rsplit(',') # This is fine, split3 indexed with [1] -get_first = split3[0] -get_middle = split3[1] -get_last = split3[-1] +good_split = '1,2,3'.split(sep=',', maxsplit=1)[-1] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=1)[0] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=2)[-1] # This is fine, we ignore cases where maxsplit > 1 +good_split = '1,2,3'.split(sep=',', maxsplit=2)[0] # This is fine, we ignore cases where maxsplit > 1 +good_split = '1,2,3'.split(sep=',', maxsplit=2)[1] # This is fine, we ignore cases where maxsplit > 1 -split1, split2 = SEQ.split(','), SEQ.rsplit(',') # [consider-using-str-partition, consider-using-str-partition] -get_first = split1[0] -get_last = split1[-1] -get_first = split2[0] -get_last = split2[-1] +## str.rsplit() tests +bad_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[1] # [consider-using-str-partition] -split1, split2 = SEQ.split(','), SEQ.rsplit(',') # This is fine, both splits are utilized -get_first = split1[0] -get_last = split1[-1] -get_first = split2[0] -get_last = split2[-1] -split1[1] = split2[1] +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[-1] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[0] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[-1] # This is fine, we ignore cases where maxsplit > 1 +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[0] # This is fine, we ignore cases where maxsplit > 1 +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[1] # This is fine, we ignore cases where maxsplit > 1 # Tests on class attributes @@ -73,10 +53,6 @@ def get_string(self) -> str: get_mid = Foo.class_str.split(',')[1] get_mid = Foo.class_str.split(',')[-2] -split2 = Foo.class_str.split(',') # [consider-using-str-partition] -get_first = split2[0] -get_last = split2[-1] - # Test with accessors bar = Foo() diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index 3d176b4ca7..cd22f32a88 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -1,25 +1,20 @@ -consider-using-str-partition:6:12::Consider using '1,2,3'.partition(',') instead -consider-using-str-partition:7:12::"Consider using '1,2,3'[::-1].partition(',') instead" -consider-using-str-partition:17:12::Consider using '1,2,3'.partition(',') instead -consider-using-str-partition:18:11::"Consider using '1,2,3'[::-1].partition(',') instead" -consider-using-str-partition:21:12::Consider using SEQ.partition(',') instead -consider-using-str-partition:22:11::Consider using SEQ.rpartition(',') instead -consider-using-str-partition:23:12::Consider using SEQ.partition(',') instead -consider-using-str-partition:24:11::Consider using SEQ.rpartition(',') instead -consider-using-str-partition:31:9::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead -consider-using-str-partition:35:9::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead -consider-using-str-partition:44:17::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead -consider-using-str-partition:44:33::Consider using SEQ.partition(',') to extract the first element of a .split() and SEQ.rpartition(',') to extract the last element of a .split() instead -consider-using-str-partition:68:12::Consider using Foo.class_str.partition(',') instead -consider-using-str-partition:69:11::Consider using Foo.class_str.rpartition(',') instead -consider-using-str-partition:70:12::Consider using Foo.class_str.partition(',') instead -consider-using-str-partition:71:11::Consider using Foo.class_str.rpartition(',') instead -consider-using-str-partition:76:9::Consider using Foo.class_str.partition(',') to extract the first element of a .split() and Foo.class_str.rpartition(',') to extract the last element of a .split() instead -consider-using-str-partition:83:12::Consider using bar.get_string().partition(',') instead -consider-using-str-partition:84:11::Consider using bar.get_string().rpartition(',') instead -consider-using-str-partition:93:10::Consider using s.partition(' ') instead -consider-using-str-partition:94:10::Consider using s.rpartition(' ') instead -consider-using-str-partition:102:6::Consider using Bar.split.partition(',') instead -consider-using-str-partition:103:6::Consider using Bar.split.rpartition(',') instead -consider-using-str-partition:104:6::Consider using Bar.split.partition(',') instead -consider-using-str-partition:105:6::Consider using Bar.split.rpartition(',') instead +consider-using-str-partition:5:12::Consider using '1,2,3'.partition(',')[0] instead +consider-using-str-partition:6:11::"Consider using '1,2,3'[::-1].partition(',')[0] instead" +consider-using-str-partition:9:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:10:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:11:12::Consider using SEQ.partition(',')[0] instead +consider-using-str-partition:12:11::Consider using SEQ.rpartition(',')[-1] instead +consider-using-str-partition:20:12::Consider using '1,2,3'.partition(,)[-1] instead +consider-using-str-partition:29:12::Consider using '1,2,3'.rpartition(,)[-1] instead +consider-using-str-partition:48:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:49:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:50:12::Consider using Foo.class_str.partition(',')[0] instead +consider-using-str-partition:51:11::Consider using Foo.class_str.rpartition(',')[-1] instead +consider-using-str-partition:59:12::Consider using bar.get_string().partition(',')[0] instead +consider-using-str-partition:60:11::Consider using bar.get_string().rpartition(',')[-1] instead +consider-using-str-partition:69:10::Consider using s.partition(' ')[0] instead +consider-using-str-partition:70:10::Consider using s.rpartition(' ')[-1] instead +consider-using-str-partition:78:6::Consider using Bar.split.partition(',')[0] instead +consider-using-str-partition:79:6::Consider using Bar.split.rpartition(',')[-1] instead +consider-using-str-partition:80:6::Consider using Bar.split.partition(',')[0] instead +consider-using-str-partition:81:6::Consider using Bar.split.rpartition(',')[-1] instead From 3c8b0f3f3f4de4bd21d3abc54e27b65f2f43868a Mon Sep 17 00:00:00 2001 From: yushao2 <36848472+yushao2@users.noreply.github.com> Date: Tue, 18 May 2021 10:39:35 +0800 Subject: [PATCH 12/26] Update ChangeLog Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com> --- ChangeLog | 3 +-- doc/whatsnew/2.9.rst | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 09c9d7ac80..8264da8959 100644 --- a/ChangeLog +++ b/ChangeLog @@ -52,8 +52,7 @@ modules are added. Closes #4412 * New checker ``consider-using-str-partition``. Emitted either when only accessing the first or last - element of a ``str.split()`` or ``str.rsplit()``, or when a ``str.split()`` or ``str.rsplit()`` is - used with a ``max_split`` argument value of 1. + element of ``str.split()``. Closes #4440 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 48c4eaa9b8..9434452970 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -16,9 +16,8 @@ New checkers indexing the same dictionary with the key within loop body. -* ``consider-using-str-partition``: Emitted when accessing only the first or last element - of a ``str.split()`` or ``str.rsplit()``, or when a ``str.split()`` or ``str.rsplit()`` is - used with a ``max_split`` argument value of 1. +* ``consider-using-str-partition``: Emitted either when only accessing the first or last + element of ``str.split()``. * An ``ignore_signatures`` option has been added to the similarity checker. It will permits to reduce false positives when multiple functions have the same parameters. From cd86ca0ed08f14d9fc992245d1706f610f4ac78c Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Tue, 18 May 2021 11:07:40 +0800 Subject: [PATCH 13/26] Updated PR from review --- .../refactoring/recommendation_checker.py | 37 ++++++++----------- pylint/checkers/utils.py | 12 +++++- 2 files changed, 27 insertions(+), 22 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 9fa493b747..135cbd7f46 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -38,12 +38,9 @@ class RecommendationChecker(checkers.BaseChecker): "C0207": ( "Consider using %s instead", "consider-using-str-partition", - "Emitted when accessing only the first or last element of a str.split(sep). " - "Or when a str.split(sep,maxsplit=1) is used. " - "The first and last element can be accessed by using str.partition(sep)[0] " - "or str.rpartition(sep)[-1] instead, which is less computationally " - "expensive and works the same as str.split() or str.rsplit() with a maxsplit " - "of 1", + "Emitted when accessing only the first or last element of str.split(). " + "The first and last element can be accessed by using str.partition()[0] " + "or str.rpartition()[-1] instead.", ), } @@ -83,16 +80,16 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: or when split/rsplit with max_split=1 is used""" # Check if call is split() or rsplit() - if isinstance(node.func, astroid.Attribute) and node.func.attrname in ( - "split", - "rsplit", + if ( + isinstance(node.func, astroid.Attribute) + and node.func.attrname + in ( + "split", + "rsplit", + ) + and isinstance(utils.safe_infer(node.func), astroid.BoundMethod) ): - inferred_func = utils.safe_infer(node.func) - fn_name = node.func.attrname - node_name = node.as_string() - if not isinstance(inferred_func, astroid.BoundMethod): - return try: seperator = utils.get_argument_from_call(node, 0, "sep").value except utils.NoSuchArgumentError: @@ -103,29 +100,27 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value if maxsplit > 1: return - except utils.NoSuchArgumentError: maxsplit = 0 # Check if it's immediately subscripted if isinstance(node.parent, astroid.Subscript): - subscript_node = node.parent # Check if subscripted with -1/0 try: - subscript_value = utils.get_subscript_const_value( - subscript_node - ).value - except ValueError: + subscript_value = utils.get_subscript_const_value(node.parent).value + except utils.InferredTypeError: return if maxsplit == 1 and subscript_value == 1: new_func = node.func.attrname.replace("split", "partition") - new_name = f"{node.as_string().rpartition('.')[0]}.{new_func}({seperator})[-1]" + new_name = f"{node.as_string().rpartition('.')[0]}.{new_func}({seperator})[2]" self.add_message( "consider-using-str-partition", node=node, args=(new_name,) ) if maxsplit == 0 and subscript_value in (-1, 0): + fn_name = node.func.attrname + node_name = node.as_string() new_fn = "rpartition" if subscript_value == -1 else "partition" new_fn = new_fn[::-1] node_name = node_name[::-1] diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py index 79b01dac76..4581c7849d 100644 --- a/pylint/checkers/utils.py +++ b/pylint/checkers/utils.py @@ -272,6 +272,10 @@ class NoSuchArgumentError(Exception): pass +class InferredTypeError(Exception): + pass + + def is_inside_except(node): """Returns true if node is inside the name of an except handler.""" current = node @@ -1529,12 +1533,18 @@ def get_subscript_const_value(node: astroid.Subscript) -> astroid.Const: Returns the value (subscript.slice) of a Subscript node, also supports python <3.9 windows where node.slice might be an Index node + + :param node: Subscript Node to extract value from + :returns: Const Node containing subscript value + :raises InferredTypeError: if the subscript node cannot be inferred as a Const """ value = node.slice if isinstance(value, astroid.Index): value = value.value inferred = safe_infer(value) if not isinstance(inferred, astroid.Const): - raise ValueError("Subscript.slice cannot be inferred as an astroid.Const") + raise InferredTypeError( + "Subscript.slice cannot be inferred as an astroid.Const" + ) return inferred From 23e82ebd7d21c3276d72d31dbbfeedd1672bc652 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Tue, 18 May 2021 11:14:29 +0800 Subject: [PATCH 14/26] Refactored if-else logic in consider-using-str-partition --- .../checkers/refactoring/recommendation_checker.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 135cbd7f46..5fe9407559 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -114,11 +114,8 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: if maxsplit == 1 and subscript_value == 1: new_func = node.func.attrname.replace("split", "partition") new_name = f"{node.as_string().rpartition('.')[0]}.{new_func}({seperator})[2]" - self.add_message( - "consider-using-str-partition", node=node, args=(new_name,) - ) - if maxsplit == 0 and subscript_value in (-1, 0): + elif maxsplit == 0 and subscript_value in (-1, 0): fn_name = node.func.attrname node_name = node.as_string() new_fn = "rpartition" if subscript_value == -1 else "partition" @@ -126,11 +123,14 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: node_name = node_name[::-1] fn_name = fn_name[::-1] new_name = f"{node_name.replace(fn_name, new_fn, 1)[::-1]}[{subscript_value}]" - self.add_message( - "consider-using-str-partition", node=node, args=(new_name,) - ) + + else: return + self.add_message( + "consider-using-str-partition", node=node, args=(new_name,) + ) + @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") def visit_for(self, node: astroid.For) -> None: self._check_consider_using_enumerate(node) From 90b9b8de2e7ca39d471c2bb93e91fe330ccb75af Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Tue, 18 May 2021 11:55:48 +0800 Subject: [PATCH 15/26] updated func tests output file --- tests/functional/c/consider/consider_using_str_partition.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt index cd22f32a88..d8d36e67c7 100644 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ b/tests/functional/c/consider/consider_using_str_partition.txt @@ -4,8 +4,8 @@ consider-using-str-partition:9:12::Consider using SEQ.partition(',')[0] instead consider-using-str-partition:10:11::Consider using SEQ.rpartition(',')[-1] instead consider-using-str-partition:11:12::Consider using SEQ.partition(',')[0] instead consider-using-str-partition:12:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:20:12::Consider using '1,2,3'.partition(,)[-1] instead -consider-using-str-partition:29:12::Consider using '1,2,3'.rpartition(,)[-1] instead +consider-using-str-partition:20:12::Consider using '1,2,3'.partition(,)[2] instead +consider-using-str-partition:29:12::Consider using '1,2,3'.rpartition(,)[2] instead consider-using-str-partition:48:12::Consider using Foo.class_str.partition(',')[0] instead consider-using-str-partition:49:11::Consider using Foo.class_str.rpartition(',')[-1] instead consider-using-str-partition:50:12::Consider using Foo.class_str.partition(',')[0] instead From 91e443b7235ad485626fdebc69c25f7b350ab00f Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Wed, 19 May 2021 22:39:09 +0800 Subject: [PATCH 16/26] Changes from code review --- ChangeLog | 2 +- doc/whatsnew/2.9.rst | 2 +- .../checkers/refactoring/recommendation_checker.py | 13 ++----------- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8264da8959..112568ea4c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -51,7 +51,7 @@ modules are added. Closes #4412 -* New checker ``consider-using-str-partition``. Emitted either when only accessing the first or last +* New checker ``consider-using-str-partition``. Emitted either when accessing only the first or last element of ``str.split()``. Closes #4440 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 9434452970..d33756d056 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -16,7 +16,7 @@ New checkers indexing the same dictionary with the key within loop body. -* ``consider-using-str-partition``: Emitted either when only accessing the first or last +* ``consider-using-str-partition``: Emitted either when accessing only the first or last element of ``str.split()``. * An ``ignore_signatures`` option has been added to the similarity checker. It will permits to reduce false positives when multiple functions have the same parameters. diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 5fe9407559..bdf2346aaf 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -82,14 +82,9 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: # Check if call is split() or rsplit() if ( isinstance(node.func, astroid.Attribute) - and node.func.attrname - in ( - "split", - "rsplit", - ) + and node.func.attrname in ("split", "rsplit") and isinstance(utils.safe_infer(node.func), astroid.BoundMethod) ): - try: seperator = utils.get_argument_from_call(node, 0, "sep").value except utils.NoSuchArgumentError: @@ -117,12 +112,8 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: elif maxsplit == 0 and subscript_value in (-1, 0): fn_name = node.func.attrname - node_name = node.as_string() new_fn = "rpartition" if subscript_value == -1 else "partition" - new_fn = new_fn[::-1] - node_name = node_name[::-1] - fn_name = fn_name[::-1] - new_name = f"{node_name.replace(fn_name, new_fn, 1)[::-1]}[{subscript_value}]" + new_name = f"{new_fn.join(node.as_string().rsplit(fn_name, maxsplit=1))}[{subscript_value}]" else: return From ad1284d684277bed1defe0765d31fbfd182dc9ec Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Thu, 20 May 2021 12:49:25 +0800 Subject: [PATCH 17/26] Reworked logic based on review --- ChangeLog | 2 +- doc/whatsnew/2.9.rst | 2 +- .../refactoring/recommendation_checker.py | 40 +++++---- .../c/consider/consider_using_maxsplit_arg.py | 77 ++++++++++++++++++ .../consider/consider_using_maxsplit_arg.txt | 18 +++++ .../consider/consider_using_str_partition.py | 81 ------------------- .../consider/consider_using_str_partition.txt | 20 ----- 7 files changed, 115 insertions(+), 125 deletions(-) create mode 100644 tests/functional/c/consider/consider_using_maxsplit_arg.py create mode 100644 tests/functional/c/consider/consider_using_maxsplit_arg.txt delete mode 100644 tests/functional/c/consider/consider_using_str_partition.py delete mode 100644 tests/functional/c/consider/consider_using_str_partition.txt diff --git a/ChangeLog b/ChangeLog index 112568ea4c..6922a6137b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -51,7 +51,7 @@ modules are added. Closes #4412 -* New checker ``consider-using-str-partition``. Emitted either when accessing only the first or last +* New checker ``consider-using-maxsplit-arg``. Emitted either when accessing only the first or last element of ``str.split()``. Closes #4440 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index d33756d056..70eda9c72f 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -16,7 +16,7 @@ New checkers indexing the same dictionary with the key within loop body. -* ``consider-using-str-partition``: Emitted either when accessing only the first or last +* ``consider-using-maxsplit-arg``: Emitted either when accessing only the first or last element of ``str.split()``. * An ``ignore_signatures`` option has been added to the similarity checker. It will permits to reduce false positives when multiple functions have the same parameters. diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index bdf2346aaf..34af374e75 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -37,10 +37,11 @@ class RecommendationChecker(checkers.BaseChecker): ), "C0207": ( "Consider using %s instead", - "consider-using-str-partition", + "consider-using-maxsplit-arg", "Emitted when accessing only the first or last element of str.split(). " - "The first and last element can be accessed by using str.partition()[0] " - "or str.rpartition()[-1] instead.", + "The first and last element can be accessed by using " + "str.split(sep,maxsplit=1)[0] or str.rpartition(sep,maxsplit=1)[-1] " + "instead.", ), } @@ -52,11 +53,11 @@ def _is_builtin(node, function): return utils.is_builtin_object(inferred) and inferred.name == function @utils.check_messages( - "consider-iterating-dictionary", "consider-using-str-partition" + "consider-iterating-dictionary", "consider-using-maxsplit-arg" ) def visit_call(self, node: astroid.Call) -> None: self._check_consider_iterating_dictionary(node) - self._check_consider_using_str_partition(node) + self._check_consider_using_maxsplit_arg(node) def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: if not isinstance(node.func, astroid.Attribute): @@ -75,7 +76,7 @@ def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: if isinstance(node.parent, (astroid.For, astroid.Comprehension)): self.add_message("consider-iterating-dictionary", node=node) - def _check_consider_using_str_partition(self, node: astroid.Call) -> None: + def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: """Add message when accessing first or last elements of a str.split() or str.rsplit(), or when split/rsplit with max_split=1 is used""" @@ -86,14 +87,14 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: and isinstance(utils.safe_infer(node.func), astroid.BoundMethod) ): try: - seperator = utils.get_argument_from_call(node, 0, "sep").value + sep = utils.get_argument_from_call(node, 0, "sep").value except utils.NoSuchArgumentError: return # Check if maxsplit is set, and ignore checking if maxsplit is > 1 try: maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value - if maxsplit > 1: + if maxsplit > 0: return except utils.NoSuchArgumentError: maxsplit = 0 @@ -106,21 +107,16 @@ def _check_consider_using_str_partition(self, node: astroid.Call) -> None: except utils.InferredTypeError: return - if maxsplit == 1 and subscript_value == 1: - new_func = node.func.attrname.replace("split", "partition") - new_name = f"{node.as_string().rpartition('.')[0]}.{new_func}({seperator})[2]" - - elif maxsplit == 0 and subscript_value in (-1, 0): + if subscript_value in (-1, 0): fn_name = node.func.attrname - new_fn = "rpartition" if subscript_value == -1 else "partition" - new_name = f"{new_fn.join(node.as_string().rsplit(fn_name, maxsplit=1))}[{subscript_value}]" - - else: - return - - self.add_message( - "consider-using-str-partition", node=node, args=(new_name,) - ) + new_fn = "rsplit" if subscript_value == -1 else "split" + new_name = ( + f"{node.as_string().rsplit(fn_name, maxsplit=1)[0]}{new_fn}" + f"(sep='{sep}',maxsplit=1)[{subscript_value}]" + ) + self.add_message( + "consider-using-maxsplit-arg", node=node, args=(new_name,) + ) @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") def visit_for(self, node: astroid.For) -> None: diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.py b/tests/functional/c/consider/consider_using_maxsplit_arg.py new file mode 100644 index 0000000000..d12d7d00e2 --- /dev/null +++ b/tests/functional/c/consider/consider_using_maxsplit_arg.py @@ -0,0 +1,77 @@ +"""Emit a message for accessing first/last element of string.split""" +# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin + +# Test subscripting .split() +get_first = '1,2,3'.split(',')[0] # [consider-using-maxsplit-arg] +get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-maxsplit-arg] + +SEQ = '1,2,3' +get_first = SEQ.split(',')[0] # [consider-using-maxsplit-arg] +get_last = SEQ.split(',')[-1] # [consider-using-maxsplit-arg] +get_first = SEQ.rsplit(',')[0] # [consider-using-maxsplit-arg] +get_last = SEQ.rsplit(',')[-1] # [consider-using-maxsplit-arg] + +get_mid = SEQ.split(',')[1] # This is okay +get_mid = SEQ.split(',')[-2] # This is okay + + +# Test varying maxsplit argument -- all these will be okay +## str.split() tests +good_split = '1,2,3'.split(sep=',', maxsplit=1)[-1] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=1)[0] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=2)[-1] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=2)[0] # This is fine +good_split = '1,2,3'.split(sep=',', maxsplit=2)[1] # This is fine + +## str.rsplit() tests +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[-1] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[0] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[-1] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[0] # This is fine +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[1] # This is fine + + +# Tests on class attributes +class Foo(): + class_str = '1,2,3' + def __init__(self): + self.my_str = '1,2,3' + + def get_string(self) -> str: + return self.my_str + +# Class attributes +get_first = Foo.class_str.split(',')[0] # [consider-using-maxsplit-arg] +get_last = Foo.class_str.split(',')[-1] # [consider-using-maxsplit-arg] +get_first = Foo.class_str.rsplit(',')[0] # [consider-using-maxsplit-arg] +get_last = Foo.class_str.rsplit(',')[-1] # [consider-using-maxsplit-arg] + +get_mid = Foo.class_str.split(',')[1] +get_mid = Foo.class_str.split(',')[-2] + + +# Test with accessors +bar = Foo() +get_first = bar.get_string().split(',')[0] # [consider-using-maxsplit-arg] +get_last = bar.get_string().split(',')[-1] # [consider-using-maxsplit-arg] + +get_mid = bar.get_string().split(',')[1] +get_mid = bar.get_string().split(',')[-2] + + +# Test with iterating over strings +list_of_strs = ["a", "b", "c", "d", "e", "f"] +for s in list_of_strs: + print(s.split(" ")[0]) # [consider-using-maxsplit-arg] + print(s.split(" ")[-1]) # [consider-using-maxsplit-arg] + print(s.split(" ")[-2]) + + +# Test warning messages (matching and replacing .split / .rsplit) +class Bar(): + split = '1,2,3' + +print(Bar.split.split(",")[0]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.partition) +print(Bar.split.split(",")[-1]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.rpartition) +print(Bar.split.rsplit(",")[0]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.partition) +print(Bar.split.rsplit(",")[-1]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.rpartition) diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.txt b/tests/functional/c/consider/consider_using_maxsplit_arg.txt new file mode 100644 index 0000000000..623c5bc3e7 --- /dev/null +++ b/tests/functional/c/consider/consider_using_maxsplit_arg.txt @@ -0,0 +1,18 @@ +consider-using-maxsplit-arg:5:12::Consider using '1,2,3'.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:6:11::"Consider using '1,2,3'[::-1].split(sep=',',maxsplit=1)[0] instead" +consider-using-maxsplit-arg:9:12::Consider using SEQ.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:10:11::Consider using SEQ.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:11:12::Consider using SEQ.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:12:11::Consider using SEQ.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:44:12::Consider using Foo.class_str.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:45:11::Consider using Foo.class_str.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:46:12::Consider using Foo.class_str.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:47:11::Consider using Foo.class_str.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:55:12::Consider using bar.get_string().split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:56:11::Consider using bar.get_string().rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:65:10::Consider using s.split(sep=' ',maxsplit=1)[0] instead +consider-using-maxsplit-arg:66:10::Consider using s.rsplit(sep=' ',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:74:6::Consider using Bar.split.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:75:6::Consider using Bar.split.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:76:6::Consider using Bar.split.split(sep=',',maxsplit=1)[0] instead +consider-using-maxsplit-arg:77:6::Consider using Bar.split.rsplit(sep=',',maxsplit=1)[-1] instead diff --git a/tests/functional/c/consider/consider_using_str_partition.py b/tests/functional/c/consider/consider_using_str_partition.py deleted file mode 100644 index f241f64412..0000000000 --- a/tests/functional/c/consider/consider_using_str_partition.py +++ /dev/null @@ -1,81 +0,0 @@ -"""Emit a message for iteration through dict keys and subscripting dict with key.""" -# pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin - -# Test subscripting .split() -get_first = '1,2,3'.split(',')[0] # [consider-using-str-partition] -get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-str-partition] - -SEQ = '1,2,3' -get_first = SEQ.split(',')[0] # [consider-using-str-partition] -get_last = SEQ.split(',')[-1] # [consider-using-str-partition] -get_first = SEQ.rsplit(',')[0] # [consider-using-str-partition] -get_last = SEQ.rsplit(',')[-1] # [consider-using-str-partition] - -get_mid = SEQ.split(',')[1] # This is okay -get_mid = SEQ.split(',')[-2] # This is okay - - -# Test varying maxsplit argument -## str.split() tests -bad_split = '1,2,3'.split(sep=',', maxsplit=1)[1] # [consider-using-str-partition] - -good_split = '1,2,3'.split(sep=',', maxsplit=1)[-1] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=1)[0] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=2)[-1] # This is fine, we ignore cases where maxsplit > 1 -good_split = '1,2,3'.split(sep=',', maxsplit=2)[0] # This is fine, we ignore cases where maxsplit > 1 -good_split = '1,2,3'.split(sep=',', maxsplit=2)[1] # This is fine, we ignore cases where maxsplit > 1 - -## str.rsplit() tests -bad_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[1] # [consider-using-str-partition] - -good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[-1] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[0] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[-1] # This is fine, we ignore cases where maxsplit > 1 -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[0] # This is fine, we ignore cases where maxsplit > 1 -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[1] # This is fine, we ignore cases where maxsplit > 1 - - -# Tests on class attributes -class Foo(): - class_str = '1,2,3' - def __init__(self): - self.my_str = '1,2,3' - - def get_string(self) -> str: - return self.my_str - -# Class attributes -get_first = Foo.class_str.split(',')[0] # [consider-using-str-partition] -get_last = Foo.class_str.split(',')[-1] # [consider-using-str-partition] -get_first = Foo.class_str.rsplit(',')[0] # [consider-using-str-partition] -get_last = Foo.class_str.rsplit(',')[-1] # [consider-using-str-partition] - -get_mid = Foo.class_str.split(',')[1] -get_mid = Foo.class_str.split(',')[-2] - - -# Test with accessors -bar = Foo() -get_first = bar.get_string().split(',')[0] # [consider-using-str-partition] -get_last = bar.get_string().split(',')[-1] # [consider-using-str-partition] - -get_mid = bar.get_string().split(',')[1] -get_mid = bar.get_string().split(',')[-2] - - -# Test with iterating over strings -list_of_strs = ["a", "b", "c", "d", "e", "f"] -for s in list_of_strs: - print(s.split(" ")[0]) # [consider-using-str-partition] - print(s.split(" ")[-1]) # [consider-using-str-partition] - print(s.split(" ")[-2]) - - -# Test warning messages (matching and replacing .split / .rsplit) -class Bar(): - split = '1,2,3' - -print(Bar.split.split(",")[0]) # [consider-using-str-partition] (Error message should show Bar.split.partition) -print(Bar.split.split(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) -print(Bar.split.rsplit(",")[0]) # [consider-using-str-partition] (Error message should show Bar.split.partition) -print(Bar.split.rsplit(",")[-1]) # [consider-using-str-partition] (Error message should show Bar.split.rpartition) diff --git a/tests/functional/c/consider/consider_using_str_partition.txt b/tests/functional/c/consider/consider_using_str_partition.txt deleted file mode 100644 index d8d36e67c7..0000000000 --- a/tests/functional/c/consider/consider_using_str_partition.txt +++ /dev/null @@ -1,20 +0,0 @@ -consider-using-str-partition:5:12::Consider using '1,2,3'.partition(',')[0] instead -consider-using-str-partition:6:11::"Consider using '1,2,3'[::-1].partition(',')[0] instead" -consider-using-str-partition:9:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:10:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:11:12::Consider using SEQ.partition(',')[0] instead -consider-using-str-partition:12:11::Consider using SEQ.rpartition(',')[-1] instead -consider-using-str-partition:20:12::Consider using '1,2,3'.partition(,)[2] instead -consider-using-str-partition:29:12::Consider using '1,2,3'.rpartition(,)[2] instead -consider-using-str-partition:48:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:49:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:50:12::Consider using Foo.class_str.partition(',')[0] instead -consider-using-str-partition:51:11::Consider using Foo.class_str.rpartition(',')[-1] instead -consider-using-str-partition:59:12::Consider using bar.get_string().partition(',')[0] instead -consider-using-str-partition:60:11::Consider using bar.get_string().rpartition(',')[-1] instead -consider-using-str-partition:69:10::Consider using s.partition(' ')[0] instead -consider-using-str-partition:70:10::Consider using s.rpartition(' ')[-1] instead -consider-using-str-partition:78:6::Consider using Bar.split.partition(',')[0] instead -consider-using-str-partition:79:6::Consider using Bar.split.rpartition(',')[-1] instead -consider-using-str-partition:80:6::Consider using Bar.split.partition(',')[0] instead -consider-using-str-partition:81:6::Consider using Bar.split.rpartition(',')[-1] instead From 99dbcafdc2b0b20d5a9aecc8f558e4113cb98636 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Fri, 21 May 2021 11:24:21 +0800 Subject: [PATCH 18/26] Reviewed PR based on comments --- .../refactoring/recommendation_checker.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 34af374e75..5d96aa3367 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -40,7 +40,7 @@ class RecommendationChecker(checkers.BaseChecker): "consider-using-maxsplit-arg", "Emitted when accessing only the first or last element of str.split(). " "The first and last element can be accessed by using " - "str.split(sep,maxsplit=1)[0] or str.rpartition(sep,maxsplit=1)[-1] " + "str.split(sep,maxsplit=1)[0] or str.rsplit(sep,maxsplit=1)[-1] " "instead.", ), } @@ -77,8 +77,7 @@ def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: self.add_message("consider-iterating-dictionary", node=node) def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: - """Add message when accessing first or last elements of a str.split() or str.rsplit(), - or when split/rsplit with max_split=1 is used""" + """Add message when accessing first or last elements of a str.split() or str.rsplit().""" # Check if call is split() or rsplit() if ( @@ -91,17 +90,14 @@ def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: except utils.NoSuchArgumentError: return - # Check if maxsplit is set, and ignore checking if maxsplit is > 1 try: - maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value - if maxsplit > 0: - return + # Ignore if maxsplit arg has been set + _ = utils.get_argument_from_call(node, 1, "maxsplit").value + return except utils.NoSuchArgumentError: - maxsplit = 0 + pass - # Check if it's immediately subscripted if isinstance(node.parent, astroid.Subscript): - # Check if subscripted with -1/0 try: subscript_value = utils.get_subscript_const_value(node.parent).value except utils.InferredTypeError: From f60042a2477112c9927d43a1121978e3d8476c72 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Fri, 21 May 2021 11:26:46 +0800 Subject: [PATCH 19/26] Edited helptext --- .../refactoring/recommendation_checker.py | 2 +- .../consider/consider_using_maxsplit_arg.txt | 36 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 5d96aa3367..42e58dc0ae 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -108,7 +108,7 @@ def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: new_fn = "rsplit" if subscript_value == -1 else "split" new_name = ( f"{node.as_string().rsplit(fn_name, maxsplit=1)[0]}{new_fn}" - f"(sep='{sep}',maxsplit=1)[{subscript_value}]" + f"('{sep}', maxsplit=1)[{subscript_value}]" ) self.add_message( "consider-using-maxsplit-arg", node=node, args=(new_name,) diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.txt b/tests/functional/c/consider/consider_using_maxsplit_arg.txt index 623c5bc3e7..c47f9d52b7 100644 --- a/tests/functional/c/consider/consider_using_maxsplit_arg.txt +++ b/tests/functional/c/consider/consider_using_maxsplit_arg.txt @@ -1,18 +1,18 @@ -consider-using-maxsplit-arg:5:12::Consider using '1,2,3'.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:6:11::"Consider using '1,2,3'[::-1].split(sep=',',maxsplit=1)[0] instead" -consider-using-maxsplit-arg:9:12::Consider using SEQ.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:10:11::Consider using SEQ.rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:11:12::Consider using SEQ.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:12:11::Consider using SEQ.rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:44:12::Consider using Foo.class_str.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:45:11::Consider using Foo.class_str.rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:46:12::Consider using Foo.class_str.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:47:11::Consider using Foo.class_str.rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:55:12::Consider using bar.get_string().split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:56:11::Consider using bar.get_string().rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:65:10::Consider using s.split(sep=' ',maxsplit=1)[0] instead -consider-using-maxsplit-arg:66:10::Consider using s.rsplit(sep=' ',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:74:6::Consider using Bar.split.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:75:6::Consider using Bar.split.rsplit(sep=',',maxsplit=1)[-1] instead -consider-using-maxsplit-arg:76:6::Consider using Bar.split.split(sep=',',maxsplit=1)[0] instead -consider-using-maxsplit-arg:77:6::Consider using Bar.split.rsplit(sep=',',maxsplit=1)[-1] instead +consider-using-maxsplit-arg:5:12::Consider using '1,2,3'.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:6:11::"Consider using '1,2,3'[::-1].split(',', maxsplit=1)[0] instead" +consider-using-maxsplit-arg:9:12::Consider using SEQ.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:10:11::Consider using SEQ.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:11:12::Consider using SEQ.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:12:11::Consider using SEQ.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:44:12::Consider using Foo.class_str.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:45:11::Consider using Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:46:12::Consider using Foo.class_str.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:47:11::Consider using Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:55:12::Consider using bar.get_string().split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:56:11::Consider using bar.get_string().rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:65:10::Consider using s.split(' ', maxsplit=1)[0] instead +consider-using-maxsplit-arg:66:10::Consider using s.rsplit(' ', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:74:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:75:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:76:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:77:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead From 35e26747ce8ac2c1ef49344b0be7adcfca94479f Mon Sep 17 00:00:00 2001 From: yushao2 <36848472+yushao2@users.noreply.github.com> Date: Fri, 21 May 2021 15:23:39 +0800 Subject: [PATCH 20/26] Update tests/functional/c/consider/consider_using_maxsplit_arg.py Co-authored-by: Pierre Sassoulas --- .../functional/c/consider/consider_using_maxsplit_arg.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.py b/tests/functional/c/consider/consider_using_maxsplit_arg.py index d12d7d00e2..8647b5417b 100644 --- a/tests/functional/c/consider/consider_using_maxsplit_arg.py +++ b/tests/functional/c/consider/consider_using_maxsplit_arg.py @@ -71,7 +71,8 @@ def get_string(self) -> str: class Bar(): split = '1,2,3' -print(Bar.split.split(",")[0]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.partition) -print(Bar.split.split(",")[-1]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.rpartition) -print(Bar.split.rsplit(",")[0]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.partition) -print(Bar.split.rsplit(",")[-1]) # [consider-using-maxsplit-arg] (Error message should show Bar.split.rpartition) +# Error message should show Bar.split.split(',', maxsplit=1) or Bar.split.rsplit(',', maxsplit=1) : +print(Bar.split.split(",")[0]) # [consider-using-maxsplit-arg] +print(Bar.split.split(",")[-1]) # [consider-using-maxsplit-arg] +print(Bar.split.rsplit(",")[0]) # [consider-using-maxsplit-arg] +print(Bar.split.rsplit(",")[-1]) # [consider-using-maxsplit-arg] From 4922651f5d76cd039477abd5d7cd91cfa20ed07d Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Fri, 21 May 2021 15:37:26 +0800 Subject: [PATCH 21/26] Updated func test results --- .../functional/c/consider/consider_using_maxsplit_arg.txt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.txt b/tests/functional/c/consider/consider_using_maxsplit_arg.txt index c47f9d52b7..43b78433e6 100644 --- a/tests/functional/c/consider/consider_using_maxsplit_arg.txt +++ b/tests/functional/c/consider/consider_using_maxsplit_arg.txt @@ -12,7 +12,7 @@ consider-using-maxsplit-arg:55:12::Consider using bar.get_string().split(',', ma consider-using-maxsplit-arg:56:11::Consider using bar.get_string().rsplit(',', maxsplit=1)[-1] instead consider-using-maxsplit-arg:65:10::Consider using s.split(' ', maxsplit=1)[0] instead consider-using-maxsplit-arg:66:10::Consider using s.rsplit(' ', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:74:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:75:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:76:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:77:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:75:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:76:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead +consider-using-maxsplit-arg:77:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead +consider-using-maxsplit-arg:78:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead From a1f333725ae94f1611d090e66140bb453ee4ffad Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Fri, 21 May 2021 19:33:22 +0800 Subject: [PATCH 22/26] Renamed check to use-maxsplit-arg --- ChangeLog | 2 +- doc/whatsnew/2.9.rst | 2 +- .../refactoring/recommendation_checker.py | 16 ++++----- .../consider/consider_using_maxsplit_arg.txt | 18 ---------- .../use/use_maxsplit_arg.py} | 36 +++++++++---------- tests/functional/u/use/use_maxsplit_arg.txt | 18 ++++++++++ 6 files changed, 44 insertions(+), 48 deletions(-) delete mode 100644 tests/functional/c/consider/consider_using_maxsplit_arg.txt rename tests/functional/{c/consider/consider_using_maxsplit_arg.py => u/use/use_maxsplit_arg.py} (58%) create mode 100644 tests/functional/u/use/use_maxsplit_arg.txt diff --git a/ChangeLog b/ChangeLog index 6922a6137b..3b712b88d5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -51,7 +51,7 @@ modules are added. Closes #4412 -* New checker ``consider-using-maxsplit-arg``. Emitted either when accessing only the first or last +* New checker ``use-maxsplit-arg``. Emitted either when accessing only the first or last element of ``str.split()``. Closes #4440 diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 70eda9c72f..82a5a9b843 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -16,7 +16,7 @@ New checkers indexing the same dictionary with the key within loop body. -* ``consider-using-maxsplit-arg``: Emitted either when accessing only the first or last +* ``use-maxsplit-arg``: Emitted either when accessing only the first or last element of ``str.split()``. * An ``ignore_signatures`` option has been added to the similarity checker. It will permits to reduce false positives when multiple functions have the same parameters. diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 42e58dc0ae..7f89f60afe 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -36,8 +36,8 @@ class RecommendationChecker(checkers.BaseChecker): "method of the dictionary instead.", ), "C0207": ( - "Consider using %s instead", - "consider-using-maxsplit-arg", + "Use %s instead", + "use-maxsplit-arg", "Emitted when accessing only the first or last element of str.split(). " "The first and last element can be accessed by using " "str.split(sep,maxsplit=1)[0] or str.rsplit(sep,maxsplit=1)[-1] " @@ -52,12 +52,10 @@ def _is_builtin(node, function): return False return utils.is_builtin_object(inferred) and inferred.name == function - @utils.check_messages( - "consider-iterating-dictionary", "consider-using-maxsplit-arg" - ) + @utils.check_messages("consider-iterating-dictionary", "use-maxsplit-arg") def visit_call(self, node: astroid.Call) -> None: self._check_consider_iterating_dictionary(node) - self._check_consider_using_maxsplit_arg(node) + self._check_use_maxsplit_arg(node) def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: if not isinstance(node.func, astroid.Attribute): @@ -76,7 +74,7 @@ def _check_consider_iterating_dictionary(self, node: astroid.Call) -> None: if isinstance(node.parent, (astroid.For, astroid.Comprehension)): self.add_message("consider-iterating-dictionary", node=node) - def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: + def _check_use_maxsplit_arg(self, node: astroid.Call) -> None: """Add message when accessing first or last elements of a str.split() or str.rsplit().""" # Check if call is split() or rsplit() @@ -110,9 +108,7 @@ def _check_consider_using_maxsplit_arg(self, node: astroid.Call) -> None: f"{node.as_string().rsplit(fn_name, maxsplit=1)[0]}{new_fn}" f"('{sep}', maxsplit=1)[{subscript_value}]" ) - self.add_message( - "consider-using-maxsplit-arg", node=node, args=(new_name,) - ) + self.add_message("use-maxsplit-arg", node=node, args=(new_name,)) @utils.check_messages("consider-using-enumerate", "consider-using-dict-items") def visit_for(self, node: astroid.For) -> None: diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.txt b/tests/functional/c/consider/consider_using_maxsplit_arg.txt deleted file mode 100644 index 43b78433e6..0000000000 --- a/tests/functional/c/consider/consider_using_maxsplit_arg.txt +++ /dev/null @@ -1,18 +0,0 @@ -consider-using-maxsplit-arg:5:12::Consider using '1,2,3'.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:6:11::"Consider using '1,2,3'[::-1].split(',', maxsplit=1)[0] instead" -consider-using-maxsplit-arg:9:12::Consider using SEQ.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:10:11::Consider using SEQ.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:11:12::Consider using SEQ.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:12:11::Consider using SEQ.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:44:12::Consider using Foo.class_str.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:45:11::Consider using Foo.class_str.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:46:12::Consider using Foo.class_str.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:47:11::Consider using Foo.class_str.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:55:12::Consider using bar.get_string().split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:56:11::Consider using bar.get_string().rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:65:10::Consider using s.split(' ', maxsplit=1)[0] instead -consider-using-maxsplit-arg:66:10::Consider using s.rsplit(' ', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:75:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:76:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead -consider-using-maxsplit-arg:77:6::Consider using Bar.split.split(',', maxsplit=1)[0] instead -consider-using-maxsplit-arg:78:6::Consider using Bar.split.rsplit(',', maxsplit=1)[-1] instead diff --git a/tests/functional/c/consider/consider_using_maxsplit_arg.py b/tests/functional/u/use/use_maxsplit_arg.py similarity index 58% rename from tests/functional/c/consider/consider_using_maxsplit_arg.py rename to tests/functional/u/use/use_maxsplit_arg.py index 8647b5417b..a2201c14a1 100644 --- a/tests/functional/c/consider/consider_using_maxsplit_arg.py +++ b/tests/functional/u/use/use_maxsplit_arg.py @@ -2,14 +2,14 @@ # pylint: disable=line-too-long,missing-docstring,unsubscriptable-object,too-few-public-methods,invalid-name,redefined-builtin # Test subscripting .split() -get_first = '1,2,3'.split(',')[0] # [consider-using-maxsplit-arg] -get_last = '1,2,3'[::-1].split(',')[0] # [consider-using-maxsplit-arg] +get_first = '1,2,3'.split(',')[0] # [use-maxsplit-arg] +get_last = '1,2,3'[::-1].split(',')[0] # [use-maxsplit-arg] SEQ = '1,2,3' -get_first = SEQ.split(',')[0] # [consider-using-maxsplit-arg] -get_last = SEQ.split(',')[-1] # [consider-using-maxsplit-arg] -get_first = SEQ.rsplit(',')[0] # [consider-using-maxsplit-arg] -get_last = SEQ.rsplit(',')[-1] # [consider-using-maxsplit-arg] +get_first = SEQ.split(',')[0] # [use-maxsplit-arg] +get_last = SEQ.split(',')[-1] # [use-maxsplit-arg] +get_first = SEQ.rsplit(',')[0] # [use-maxsplit-arg] +get_last = SEQ.rsplit(',')[-1] # [use-maxsplit-arg] get_mid = SEQ.split(',')[1] # This is okay get_mid = SEQ.split(',')[-2] # This is okay @@ -41,10 +41,10 @@ def get_string(self) -> str: return self.my_str # Class attributes -get_first = Foo.class_str.split(',')[0] # [consider-using-maxsplit-arg] -get_last = Foo.class_str.split(',')[-1] # [consider-using-maxsplit-arg] -get_first = Foo.class_str.rsplit(',')[0] # [consider-using-maxsplit-arg] -get_last = Foo.class_str.rsplit(',')[-1] # [consider-using-maxsplit-arg] +get_first = Foo.class_str.split(',')[0] # [use-maxsplit-arg] +get_last = Foo.class_str.split(',')[-1] # [use-maxsplit-arg] +get_first = Foo.class_str.rsplit(',')[0] # [use-maxsplit-arg] +get_last = Foo.class_str.rsplit(',')[-1] # [use-maxsplit-arg] get_mid = Foo.class_str.split(',')[1] get_mid = Foo.class_str.split(',')[-2] @@ -52,8 +52,8 @@ def get_string(self) -> str: # Test with accessors bar = Foo() -get_first = bar.get_string().split(',')[0] # [consider-using-maxsplit-arg] -get_last = bar.get_string().split(',')[-1] # [consider-using-maxsplit-arg] +get_first = bar.get_string().split(',')[0] # [use-maxsplit-arg] +get_last = bar.get_string().split(',')[-1] # [use-maxsplit-arg] get_mid = bar.get_string().split(',')[1] get_mid = bar.get_string().split(',')[-2] @@ -62,8 +62,8 @@ def get_string(self) -> str: # Test with iterating over strings list_of_strs = ["a", "b", "c", "d", "e", "f"] for s in list_of_strs: - print(s.split(" ")[0]) # [consider-using-maxsplit-arg] - print(s.split(" ")[-1]) # [consider-using-maxsplit-arg] + print(s.split(" ")[0]) # [use-maxsplit-arg] + print(s.split(" ")[-1]) # [use-maxsplit-arg] print(s.split(" ")[-2]) @@ -72,7 +72,7 @@ class Bar(): split = '1,2,3' # Error message should show Bar.split.split(',', maxsplit=1) or Bar.split.rsplit(',', maxsplit=1) : -print(Bar.split.split(",")[0]) # [consider-using-maxsplit-arg] -print(Bar.split.split(",")[-1]) # [consider-using-maxsplit-arg] -print(Bar.split.rsplit(",")[0]) # [consider-using-maxsplit-arg] -print(Bar.split.rsplit(",")[-1]) # [consider-using-maxsplit-arg] +print(Bar.split.split(",")[0]) # [use-maxsplit-arg] +print(Bar.split.split(",")[-1]) # [use-maxsplit-arg] +print(Bar.split.rsplit(",")[0]) # [use-maxsplit-arg] +print(Bar.split.rsplit(",")[-1]) # [use-maxsplit-arg] diff --git a/tests/functional/u/use/use_maxsplit_arg.txt b/tests/functional/u/use/use_maxsplit_arg.txt new file mode 100644 index 0000000000..e336bbd970 --- /dev/null +++ b/tests/functional/u/use/use_maxsplit_arg.txt @@ -0,0 +1,18 @@ +use-maxsplit-arg:5:12::Use '1,2,3'.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:6:11::"Use '1,2,3'[::-1].split(',', maxsplit=1)[0] instead" +use-maxsplit-arg:9:12::Use SEQ.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:10:11::Use SEQ.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:11:12::Use SEQ.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:12:11::Use SEQ.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:44:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:45:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:46:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:47:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:55:12::Use bar.get_string().split(',', maxsplit=1)[0] instead +use-maxsplit-arg:56:11::Use bar.get_string().rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:65:10::Use s.split(' ', maxsplit=1)[0] instead +use-maxsplit-arg:66:10::Use s.rsplit(' ', maxsplit=1)[-1] instead +use-maxsplit-arg:75:6::Use Bar.split.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:76:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:77:6::Use Bar.split.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:78:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead From d46b8f1830caac9b7ebed4193f251a20b8ff65c7 Mon Sep 17 00:00:00 2001 From: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Sat, 22 May 2021 00:12:24 +0200 Subject: [PATCH 23/26] Small formatting changes --- doc/whatsnew/2.9.rst | 1 - .../refactoring/recommendation_checker.py | 2 +- tests/functional/u/use/use_maxsplit_arg.py | 31 ++++++++++--------- tests/functional/u/use/use_maxsplit_arg.txt | 24 +++++++------- 4 files changed, 29 insertions(+), 29 deletions(-) diff --git a/doc/whatsnew/2.9.rst b/doc/whatsnew/2.9.rst index 82a5a9b843..4e6249a9a4 100644 --- a/doc/whatsnew/2.9.rst +++ b/doc/whatsnew/2.9.rst @@ -15,7 +15,6 @@ New checkers * ``consider-using-dict-items``: Emitted when iterating over dictionary keys and then indexing the same dictionary with the key within loop body. - * ``use-maxsplit-arg``: Emitted either when accessing only the first or last element of ``str.split()``. diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 7f89f60afe..30c0b091a7 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -40,7 +40,7 @@ class RecommendationChecker(checkers.BaseChecker): "use-maxsplit-arg", "Emitted when accessing only the first or last element of str.split(). " "The first and last element can be accessed by using " - "str.split(sep,maxsplit=1)[0] or str.rsplit(sep,maxsplit=1)[-1] " + "str.split(sep, maxsplit=1)[0] or str.rsplit(sep, maxsplit=1)[-1] " "instead.", ), } diff --git a/tests/functional/u/use/use_maxsplit_arg.py b/tests/functional/u/use/use_maxsplit_arg.py index a2201c14a1..cb6eb2bb6f 100644 --- a/tests/functional/u/use/use_maxsplit_arg.py +++ b/tests/functional/u/use/use_maxsplit_arg.py @@ -11,24 +11,25 @@ get_first = SEQ.rsplit(',')[0] # [use-maxsplit-arg] get_last = SEQ.rsplit(',')[-1] # [use-maxsplit-arg] -get_mid = SEQ.split(',')[1] # This is okay -get_mid = SEQ.split(',')[-2] # This is okay +# Don't suggest maxsplit=1 if not accessing the first or last element +get_mid = SEQ.split(',')[1] +get_mid = SEQ.split(',')[-2] # Test varying maxsplit argument -- all these will be okay -## str.split() tests -good_split = '1,2,3'.split(sep=',', maxsplit=1)[-1] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=1)[0] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=2)[-1] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=2)[0] # This is fine -good_split = '1,2,3'.split(sep=',', maxsplit=2)[1] # This is fine +# ## str.split() tests +good_split = '1,2,3'.split(sep=',', maxsplit=1)[-1] +good_split = '1,2,3'.split(sep=',', maxsplit=1)[0] +good_split = '1,2,3'.split(sep=',', maxsplit=2)[-1] +good_split = '1,2,3'.split(sep=',', maxsplit=2)[0] +good_split = '1,2,3'.split(sep=',', maxsplit=2)[1] -## str.rsplit() tests -good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[-1] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[0] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[-1] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[0] # This is fine -good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[1] # This is fine +# ## str.rsplit() tests +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[-1] +good_split = '1,2,3'.rsplit(sep=',', maxsplit=1)[0] +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[-1] +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[0] +good_split = '1,2,3'.rsplit(sep=',', maxsplit=2)[1] # Tests on class attributes @@ -71,7 +72,7 @@ def get_string(self) -> str: class Bar(): split = '1,2,3' -# Error message should show Bar.split.split(',', maxsplit=1) or Bar.split.rsplit(',', maxsplit=1) : +# Error message should show Bar.split.split(',', maxsplit=1) or Bar.split.rsplit(',', maxsplit=1) print(Bar.split.split(",")[0]) # [use-maxsplit-arg] print(Bar.split.split(",")[-1]) # [use-maxsplit-arg] print(Bar.split.rsplit(",")[0]) # [use-maxsplit-arg] diff --git a/tests/functional/u/use/use_maxsplit_arg.txt b/tests/functional/u/use/use_maxsplit_arg.txt index e336bbd970..bf645e0d3f 100644 --- a/tests/functional/u/use/use_maxsplit_arg.txt +++ b/tests/functional/u/use/use_maxsplit_arg.txt @@ -4,15 +4,15 @@ use-maxsplit-arg:9:12::Use SEQ.split(',', maxsplit=1)[0] instead use-maxsplit-arg:10:11::Use SEQ.rsplit(',', maxsplit=1)[-1] instead use-maxsplit-arg:11:12::Use SEQ.split(',', maxsplit=1)[0] instead use-maxsplit-arg:12:11::Use SEQ.rsplit(',', maxsplit=1)[-1] instead -use-maxsplit-arg:44:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead -use-maxsplit-arg:45:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead -use-maxsplit-arg:46:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead -use-maxsplit-arg:47:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead -use-maxsplit-arg:55:12::Use bar.get_string().split(',', maxsplit=1)[0] instead -use-maxsplit-arg:56:11::Use bar.get_string().rsplit(',', maxsplit=1)[-1] instead -use-maxsplit-arg:65:10::Use s.split(' ', maxsplit=1)[0] instead -use-maxsplit-arg:66:10::Use s.rsplit(' ', maxsplit=1)[-1] instead -use-maxsplit-arg:75:6::Use Bar.split.split(',', maxsplit=1)[0] instead -use-maxsplit-arg:76:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead -use-maxsplit-arg:77:6::Use Bar.split.split(',', maxsplit=1)[0] instead -use-maxsplit-arg:78:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:45:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:46:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:47:12::Use Foo.class_str.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:48:11::Use Foo.class_str.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:56:12::Use bar.get_string().split(',', maxsplit=1)[0] instead +use-maxsplit-arg:57:11::Use bar.get_string().rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:66:10::Use s.split(' ', maxsplit=1)[0] instead +use-maxsplit-arg:67:10::Use s.rsplit(' ', maxsplit=1)[-1] instead +use-maxsplit-arg:76:6::Use Bar.split.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:77:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:78:6::Use Bar.split.split(',', maxsplit=1)[0] instead +use-maxsplit-arg:79:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead From 22824dd8bcd18c46fb82b4a92bf236a97a2cb3c6 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Sat, 22 May 2021 10:57:03 +0800 Subject: [PATCH 24/26] Fixed escaping special characters issue --- pylint/checkers/refactoring/recommendation_checker.py | 6 +++--- tests/functional/u/use/use_maxsplit_arg.py | 3 +++ tests/functional/u/use/use_maxsplit_arg.txt | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 30c0b091a7..1c240b2d30 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -84,7 +84,7 @@ def _check_use_maxsplit_arg(self, node: astroid.Call) -> None: and isinstance(utils.safe_infer(node.func), astroid.BoundMethod) ): try: - sep = utils.get_argument_from_call(node, 0, "sep").value + utils.get_argument_from_call(node, 0, "sep").value except utils.NoSuchArgumentError: return @@ -105,8 +105,8 @@ def _check_use_maxsplit_arg(self, node: astroid.Call) -> None: fn_name = node.func.attrname new_fn = "rsplit" if subscript_value == -1 else "split" new_name = ( - f"{node.as_string().rsplit(fn_name, maxsplit=1)[0]}{new_fn}" - f"('{sep}', maxsplit=1)[{subscript_value}]" + new_fn.join(node.as_string().rsplit(fn_name, maxsplit=1))[:-1] + + f", maxsplit=1)[{subscript_value}]" ) self.add_message("use-maxsplit-arg", node=node, args=(new_name,)) diff --git a/tests/functional/u/use/use_maxsplit_arg.py b/tests/functional/u/use/use_maxsplit_arg.py index cb6eb2bb6f..6850312110 100644 --- a/tests/functional/u/use/use_maxsplit_arg.py +++ b/tests/functional/u/use/use_maxsplit_arg.py @@ -77,3 +77,6 @@ class Bar(): print(Bar.split.split(",")[-1]) # [use-maxsplit-arg] print(Bar.split.rsplit(",")[0]) # [use-maxsplit-arg] print(Bar.split.rsplit(",")[-1]) # [use-maxsplit-arg] + + +a = "1,2,3".split('\n')[0] # [use-maxsplit-arg] diff --git a/tests/functional/u/use/use_maxsplit_arg.txt b/tests/functional/u/use/use_maxsplit_arg.txt index bf645e0d3f..b1cf3d66bb 100644 --- a/tests/functional/u/use/use_maxsplit_arg.txt +++ b/tests/functional/u/use/use_maxsplit_arg.txt @@ -16,3 +16,4 @@ use-maxsplit-arg:76:6::Use Bar.split.split(',', maxsplit=1)[0] instead use-maxsplit-arg:77:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead use-maxsplit-arg:78:6::Use Bar.split.split(',', maxsplit=1)[0] instead use-maxsplit-arg:79:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead +use-maxsplit-arg:82:4::Use '1,2,3'.split('\n', maxsplit=1)[0] instead From 100b3410ba64b50416a2e1024d1a28918a8c6697 Mon Sep 17 00:00:00 2001 From: Pang Yu Shao Date: Sat, 22 May 2021 20:46:02 +0800 Subject: [PATCH 25/26] Edited logic for helptext based on PR review comments --- pylint/checkers/refactoring/recommendation_checker.py | 5 +++-- tests/functional/u/use/use_maxsplit_arg.py | 4 +++- tests/functional/u/use/use_maxsplit_arg.txt | 2 ++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 1c240b2d30..2707c3cc62 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -105,8 +105,9 @@ def _check_use_maxsplit_arg(self, node: astroid.Call) -> None: fn_name = node.func.attrname new_fn = "rsplit" if subscript_value == -1 else "split" new_name = ( - new_fn.join(node.as_string().rsplit(fn_name, maxsplit=1))[:-1] - + f", maxsplit=1)[{subscript_value}]" + node.func.as_string().rsplit(fn_name, maxsplit=1)[0] + + new_fn + + f"({node.args[0].as_string()}, maxsplit=1)[{subscript_value}]" ) self.add_message("use-maxsplit-arg", node=node, args=(new_name,)) diff --git a/tests/functional/u/use/use_maxsplit_arg.py b/tests/functional/u/use/use_maxsplit_arg.py index 6850312110..1b998e5e92 100644 --- a/tests/functional/u/use/use_maxsplit_arg.py +++ b/tests/functional/u/use/use_maxsplit_arg.py @@ -78,5 +78,7 @@ class Bar(): print(Bar.split.rsplit(",")[0]) # [use-maxsplit-arg] print(Bar.split.rsplit(",")[-1]) # [use-maxsplit-arg] - +# Special cases a = "1,2,3".split('\n')[0] # [use-maxsplit-arg] +a = "1,2,3".split('split')[-1] # [use-maxsplit-arg] +a = "1,2,3".rsplit('rsplit')[0] # [use-maxsplit-arg] diff --git a/tests/functional/u/use/use_maxsplit_arg.txt b/tests/functional/u/use/use_maxsplit_arg.txt index b1cf3d66bb..8137374aa9 100644 --- a/tests/functional/u/use/use_maxsplit_arg.txt +++ b/tests/functional/u/use/use_maxsplit_arg.txt @@ -17,3 +17,5 @@ use-maxsplit-arg:77:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead use-maxsplit-arg:78:6::Use Bar.split.split(',', maxsplit=1)[0] instead use-maxsplit-arg:79:6::Use Bar.split.rsplit(',', maxsplit=1)[-1] instead use-maxsplit-arg:82:4::Use '1,2,3'.split('\n', maxsplit=1)[0] instead +use-maxsplit-arg:83:4::Use '1,2,3'.rsplit('split', maxsplit=1)[-1] instead +use-maxsplit-arg:84:4::Use '1,2,3'.split('rsplit', maxsplit=1)[0] instead From 7e640f5d926282cab5c43f436b3c45b9a0a59d59 Mon Sep 17 00:00:00 2001 From: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Sat, 22 May 2021 15:30:25 +0200 Subject: [PATCH 26/26] Last changes --- pylint/checkers/refactoring/recommendation_checker.py | 4 ++-- pylint/testutils/lint_module_test.py | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/recommendation_checker.py b/pylint/checkers/refactoring/recommendation_checker.py index 2707c3cc62..a21864e8d9 100644 --- a/pylint/checkers/refactoring/recommendation_checker.py +++ b/pylint/checkers/refactoring/recommendation_checker.py @@ -84,13 +84,13 @@ def _check_use_maxsplit_arg(self, node: astroid.Call) -> None: and isinstance(utils.safe_infer(node.func), astroid.BoundMethod) ): try: - utils.get_argument_from_call(node, 0, "sep").value + utils.get_argument_from_call(node, 0, "sep") except utils.NoSuchArgumentError: return try: # Ignore if maxsplit arg has been set - _ = utils.get_argument_from_call(node, 1, "maxsplit").value + utils.get_argument_from_call(node, 1, "maxsplit") return except utils.NoSuchArgumentError: pass diff --git a/pylint/testutils/lint_module_test.py b/pylint/testutils/lint_module_test.py index c970be92ab..a765381b6b 100644 --- a/pylint/testutils/lint_module_test.py +++ b/pylint/testutils/lint_module_test.py @@ -53,7 +53,8 @@ def __init__(self, test_file: FunctionalTestFile, config: Optional[Config] = Non def setUp(self): if self._should_be_skipped_due_to_version(): pytest.skip( - "Test cannot run with Python %s." % sys.version.partition(" ")[0] + "Test cannot run with Python %s." + % sys.version.split(" ", maxsplit=1)[0] ) missing = [] for requirement in self._test_file.options["requires"]: