Skip to content

Commit

Permalink
Reworked logic based on review
Browse files Browse the repository at this point in the history
  • Loading branch information
yushao2 committed May 20, 2021
1 parent 91e443b commit ad1284d
Show file tree
Hide file tree
Showing 7 changed files with 115 additions and 125 deletions.
2 changes: 1 addition & 1 deletion ChangeLog
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion doc/whatsnew/2.9.rst
Expand Up @@ -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.
Expand Down
40 changes: 18 additions & 22 deletions pylint/checkers/refactoring/recommendation_checker.py
Expand Up @@ -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] "

This comment has been minimized.

Copy link
@Pierre-Sassoulas

Pierre-Sassoulas May 20, 2021

Member

Did you mean rsplit instead of rpartition on the second example ?

"instead.",
),
}

Expand All @@ -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):
Expand All @@ -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"""

Expand All @@ -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
Expand All @@ -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:
Expand Down
77 changes: 77 additions & 0 deletions 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)
18 changes: 18 additions & 0 deletions 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
81 changes: 0 additions & 81 deletions tests/functional/c/consider/consider_using_str_partition.py

This file was deleted.

20 changes: 0 additions & 20 deletions tests/functional/c/consider/consider_using_str_partition.txt

This file was deleted.

0 comments on commit ad1284d

Please sign in to comment.