Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Implemented new check use-maxsplit-arg #4469

Merged
merged 27 commits into from May 22, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
94cc989
Implemented new check ``consider-using-str-partition``
yushao2 May 12, 2021
3173a45
Changed source of lint_module_test due to new checkers breaking
yushao2 May 12, 2021
3b36d2a
Improved help text for ``consider-using-str-partition``
yushao2 May 12, 2021
cc61bf5
Added additional tests
yushao2 May 12, 2021
eeb45d6
Improved on help text logic and accompanying tests
yushao2 May 12, 2021
943bd58
Update docs
yushao2 May 12, 2021
5963aac
Reworked consider-using-str-partition
yushao2 May 13, 2021
64cba0c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 13, 2021
860784e
Made checking subscript compatible with < py3.9
yushao2 May 13, 2021
35854ad
Refactored subscript checking to a utils function
yushao2 May 13, 2021
e4b31b6
Reworked and simplified ``consider-using-str-partition``
yushao2 May 14, 2021
47c8d41
Merge branch 'master' into checkers-4440
Pierre-Sassoulas May 17, 2021
3c8b0f3
Update ChangeLog
yushao2 May 18, 2021
cd86ca0
Updated PR from review
yushao2 May 18, 2021
23e82eb
Refactored if-else logic in consider-using-str-partition
yushao2 May 18, 2021
90b9b8d
updated func tests output file
yushao2 May 18, 2021
91e443b
Changes from code review
yushao2 May 19, 2021
ad1284d
Reworked logic based on review
yushao2 May 20, 2021
99dbcaf
Reviewed PR based on comments
yushao2 May 21, 2021
f60042a
Edited helptext
yushao2 May 21, 2021
35e2674
Update tests/functional/c/consider/consider_using_maxsplit_arg.py
yushao2 May 21, 2021
4922651
Updated func test results
yushao2 May 21, 2021
a1f3337
Renamed check to use-maxsplit-arg
yushao2 May 21, 2021
d46b8f1
Small formatting changes
cdce8p May 21, 2021
22824dd
Fixed escaping special characters issue
yushao2 May 22, 2021
100b341
Edited logic for helptext based on PR review comments
yushao2 May 22, 2021
7e640f5
Last changes
cdce8p May 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
Expand Up @@ -51,6 +51,11 @@ modules are added.

Closes #4412

* New checker ``consider-using-maxsplit-arg``. Emitted either when accessing only the first or last
element of ``str.split()``.

Closes #4440

* Add ignore_signatures to duplicate code checker

Closes #3619
Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.9.rst
Expand Up @@ -15,8 +15,13 @@ 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-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.


Other Changes
=============

Expand Down
60 changes: 58 additions & 2 deletions pylint/checkers/refactoring/recommendation_checker.py
Expand Up @@ -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 %s instead",
"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] "
yushao2 marked this conversation as resolved.
Show resolved Hide resolved
"instead.",
),
}

@staticmethod
Expand All @@ -44,8 +52,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-maxsplit-arg"
)
def visit_call(self, node: astroid.Call) -> None:
self._check_consider_iterating_dictionary(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):
return
if node.func.attrname != "keys":
Expand All @@ -62,6 +76,48 @@ 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_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"""
yushao2 marked this conversation as resolved.
Show resolved Hide resolved

# Check if call is split() or rsplit()
if (
isinstance(node.func, astroid.Attribute)
and node.func.attrname in ("split", "rsplit")
and isinstance(utils.safe_infer(node.func), astroid.BoundMethod)
):
try:
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
yushao2 marked this conversation as resolved.
Show resolved Hide resolved
try:
maxsplit = utils.get_argument_from_call(node, 1, "maxsplit").value
if maxsplit > 0:
return
except utils.NoSuchArgumentError:
maxsplit = 0
yushao2 marked this conversation as resolved.
Show resolved Hide resolved

# Check if it's immediately subscripted
if isinstance(node.parent, astroid.Subscript):
# Check if subscripted with -1/0
yushao2 marked this conversation as resolved.
Show resolved Hide resolved
try:
subscript_value = utils.get_subscript_const_value(node.parent).value
except utils.InferredTypeError:
return

if subscript_value in (-1, 0):
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='{sep}',maxsplit=1)[{subscript_value}]"
yushao2 marked this conversation as resolved.
Show resolved Hide resolved
)
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:
self._check_consider_using_enumerate(node)
Expand Down
26 changes: 26 additions & 0 deletions pylint/checkers/utils.py
Expand Up @@ -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
Expand Down Expand Up @@ -1522,3 +1526,25 @@ 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

: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 InferredTypeError(
"Subscript.slice cannot be inferred as an astroid.Const"
)

return inferred
4 changes: 3 additions & 1 deletion pylint/testutils/lint_module_test.py
Expand Up @@ -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:
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)
yushao2 marked this conversation as resolved.
Show resolved Hide resolved
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