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

[#212] Forbid variables self reassignment #262

Merged
merged 8 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
improved self variable reassigning recognition
  • Loading branch information
Roxe322 committed Oct 17, 2018
commit f2a091e8765ab31cb4a3ddf852c6e1101df1df05
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# -*- coding: utf-8 -*-

import pytest

from wemake_python_styleguide.violations.best_practices import (
ReassigningVariableToItselfViolation,
)
Expand All @@ -17,12 +19,34 @@
test_variable = 10
"""

wrong_fragment_double_assignment = """
test_variable = 5
test_variable = test_variable = 10
"""

wrong_fragment_other_assignment = """
test_variable = 5
test_variable = other = test_variable = 5
"""

wrong_fragment_tuple_assignment = """
x = 1
y = 2
x, y = x, y
Copy link
Member

@sobolevn sobolevn Oct 17, 2018

Choose a reason for hiding this comment

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

One more thing. Can you please test this case:

x = 1
y = 2
x, y = y, x

This should be a perfectly valid case. But, I am not sure if that is working right now.

"""


@pytest.mark.parametrize('fragment', [wrong_fragment,
Copy link
Member

Choose a reason for hiding this comment

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

We use different style for this kind of things: please, see

@pytest.mark.parametrize('code', [
calling_function,
called_function,
calling_method,
called_method,
accessing_attribute,
accessed_attribute,
raising_variable,
returning_variable,
awaiting_variable,
yielding_variable,
])

wrong_fragment_double_assignment,
wrong_fragment_other_assignment,
wrong_fragment_tuple_assignment,
],
)
def test_self_variable_reassignment(
Copy link
Member

Choose a reason for hiding this comment

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

Please, also check this example:

x, y = x, y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

assert_errors, parse_ast_tree, default_options,
assert_errors, parse_ast_tree, fragment, default_options,
Copy link
Member

Choose a reason for hiding this comment

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

We use code variable name for code samples, not fragment. Please, rename it.

):
"""Testing that self variable reassignment is restricted."""
tree = parse_ast_tree(wrong_fragment)
tree = parse_ast_tree(fragment)
visitor = WrongVariableAssignmentVisitor(default_options, tree=tree)
visitor.run()
assert_errors(visitor, [ReassigningVariableToItselfViolation])
Expand Down
34 changes: 17 additions & 17 deletions wemake_python_styleguide/violations/best_practices.py
Original file line number Diff line number Diff line change
Expand Up @@ -621,22 +621,6 @@ class RedundantForElseViolation(ASTViolation):
code = 436


class ReassigningVariableToItselfViolation(ASTViolation):
"""
Forbids to assign variable to itself.

Reasoning:
There is no need to do that.

Note:
Returns Z437 as error code
"""

#: Error message shown to the user.
error_template = 'Found reassigning variable to itself'
code = 437


class RedundantFinallyViolation(ASTViolation):
"""
Forbids to use ``finally`` in ``try`` block without ``except`` block.
Expand All @@ -662,10 +646,26 @@ class RedundantFinallyViolation(ASTViolation):
f.close()

Note:
Returns Z438 as error code
Returns Z437 as error code

"""

#: Error message shown to the user.
error_template = 'Found `finally` in `try` block without `except`'
code = 437


class ReassigningVariableToItselfViolation(ASTViolation):
"""
Forbids to assign variable to itself.

Reasoning:
There is no need to do that.

Note:
Returns Z438 as error code
"""

#: Error message shown to the user.
error_template = 'Found reassigning variable to itself'
Copy link
Member

Choose a reason for hiding this comment

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

Can you please provide the name of the variable here? Currently you miss '{0}' in template string

code = 438
22 changes: 18 additions & 4 deletions wemake_python_styleguide/visitors/ast/naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,24 @@ class WrongVariableAssignmentVisitor(BaseNodeVisitor):
"""Finds wrong variables assignments."""

def _check_assignment(self, node: ast.Assign) -> None:
node_value_id = getattr(node.value, 'id', None)
for target_node in node.targets:
target_node_id = getattr(target_node, 'id', None)
if target_node_id == node_value_id:
target_names = [target.id for target in node.targets
if isinstance(target, ast.Name)]
if target_names:
if getattr(node.value, 'id', None) in target_names or (
Copy link
Member

Choose a reason for hiding this comment

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

Each step in this if should be a separate variable:

taget_name = getattr(node.value, 'id', None)
PLEASE_FIND_CORRECT_NAME = len(target_names) != len(set(target_names))
if target_name in target_names or PLEASE_FIND_CORRECT_NAME: ...

Replace PLEASE_FIND_CORRECT_NAME with anything you find appropriate.

len(target_names) != len(list(set(target_names)))):
self.add_violation(ReassigningVariableToItselfViolation(node))
else:
target_names = [getattr(target, 'elts', None) for target
in node.targets if isinstance(target, ast.Tuple)]
for index in range(len(target_names)):
Copy link
Member

Choose a reason for hiding this comment

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

Use enumerate instead. range(len()) is an anti-pattern.

target_names[index] = [name.id for name in target_names[index]]
node_values = []
if isinstance(node.value, ast.Tuple):
node_values = [getattr(node_value, 'id', None) for node_value
in node.value.elts]
Copy link
Member

Choose a reason for hiding this comment

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

Please, do not abuse line breaking like this. This linter currently unable to catch these things. But, I will be more than happy to create new rules to forbid this kind of line breaks.

if node_values in target_names or (len(target_names) !=
len(list(set(target_names)))
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to cast set to list.

):
self.add_violation(ReassigningVariableToItselfViolation(node))

def visit_Assign(self, node: ast.Assign) -> None:
Expand Down
1 change: 1 addition & 0 deletions wemake_python_styleguide/visitors/presets/general.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

naming.WrongNameVisitor,
naming.WrongModuleMetadataVisitor,
naming.WrongVariableAssignmentVisitor,

numbers.MagicNumberVisitor,
WrongStringVisitor,
Expand Down