Skip to content

Commit

Permalink
Only accept simple for-loop bodies when looking for consider-using-join
Browse files Browse the repository at this point in the history
Also verify that the assignment targets are actually AssignName

Close #2250
  • Loading branch information
PCManticore committed Jul 4, 2018
1 parent bceb1d6 commit 5035722
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 5 deletions.
10 changes: 5 additions & 5 deletions pylint/checkers/refactoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -724,20 +724,20 @@ def _check_consider_using_join(self, aug_assign):
result += number # aug_assign
"""
for_loop = aug_assign.parent
if not isinstance(for_loop, astroid.node_classes.For):
if not isinstance(for_loop, astroid.For) or len(for_loop.body) > 1:
return
assign = for_loop.previous_sibling()
if not isinstance(assign, astroid.node_classes.Assign):
if not isinstance(assign, astroid.Assign):
return
result_assign_names = {target.name for target in assign.targets}
result_assign_names = {target.name for target in assign.targets if isinstance(target, astroid.AssignName)}

is_concat_loop = (aug_assign.op == '+='
and isinstance(aug_assign.target, astroid.AssignName)
and len(for_loop.body) == 1
and aug_assign.target.name in result_assign_names
and isinstance(assign.value, astroid.node_classes.Const)
and isinstance(assign.value, astroid.Const)
and isinstance(assign.value.value, str)
and isinstance(aug_assign.value, astroid.node_classes.Name)
and isinstance(aug_assign.value, astroid.Name)
and aug_assign.value.name == for_loop.target.name)
if is_concat_loop:
self.add_message('consider-using-join', node=aug_assign)
Expand Down
12 changes: 12 additions & 0 deletions pylint/test/functional/consider_join.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,15 @@
result = ''
for number in ['1']:
result.result += number

# Does not emit if the body is more complex
result = {'context': 1}
result['context'] = 0
for number in ['1']:
result1 = 42 + int(number)
result['context'] += result1 * 24

# Does not crash if the previous sibling does not have AssignNames
result['context'] = 0
for number in ['1']:
result['context'] += 24

0 comments on commit 5035722

Please sign in to comment.