[#212] Forbid variables self reassignment #262

Roxe322 · 2018-10-15T11:03:49Z

I have made things!

[V] I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
[V] I have created at least one test case for the changes I have made
[V] I have updated the documentation for the changes I have made
[V] I have added my changes to the CHANGELOG.md

Related issues

Closes: #212

sobolevn

Thanks!

sobolevn · 2018-10-15T11:07:26Z

tests/test_visitors/test_ast/test_naming/test_variable_self_reassignment.py

+    WrongVariableAssignmentVisitor,
+)
+
+wrong_fragment = """


Can you please also test this example:

test_variable = 5 test_variable = test_variable = 5

and

test_variable = 5 test_variable = other = test_variable = 5

All these examples should raise a violation.

sobolevn · 2018-10-15T11:11:14Z

@Roxe322 please, also fix merge conflicts.

sobolevn · 2018-10-15T11:14:03Z

tests/test_visitors/test_ast/test_naming/test_variable_self_reassignment.py

+"""
+
+
+def test_self_variable_reassignment(


Please, also check this example:

x, y = x, y

Roxe322 · 2018-10-17T11:04:23Z

Ooops, some fails, I need to check it

Roxe322 · 2018-10-17T11:12:01Z

Can you please explain me what is going wrong there? In my local machine pytest passing successfully :(

sobolevn

@Roxe322 thanks for your time and effort.

This code has a lot of major problems, that are hard to address in a single review. However, I will point some things that I find the most important:

Some code looks pretty identical and can be moved to the function of its own, like:

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/naming.py

Lines 178 to 179 in f2a091e

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

and

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/naming.py

Lines 190 to 192 in f2a091e

    
           if node_values in target_names or (len(target_names) != 
        
                                              len(list(set(target_names))) 
        
                                              ):

I am not sure what are you trying to do in these lines:

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/naming.py

Lines 182 to 185 in f2a091e

    
           target_names = [getattr(target, 'elts', None) for target 
        
                           in node.targets if isinstance(target, ast.Tuple)] 
        
           for index in range(len(target_names)): 
        
               target_names[index] = [name.id for name in target_names[index]]

Seems like a wrong data structure / algorithm to me

Try to adapt to the code style we have in a project. You won't find a single other example of neither of these things:

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/naming.py

Lines 190 to 192 in f2a091e

    
           if node_values in target_names or (len(target_names) != 
        
                                              len(list(set(target_names))) 
        
                                              ):

and

wemake-python-styleguide/wemake_python_styleguide/visitors/ast/naming.py

Lines 182 to 183 in f2a091e

    
           target_names = [getattr(target, 'elts', None) for target 
        
                           in node.targets if isinstance(target, ast.Tuple)]

We use different line break rules. And it is important.

I have asked @AlwxSin to help you with this PR.

Thanks again for your hard work! I would be happy to merge this PR as soon as it will be refactored.

sobolevn · 2018-10-17T11:22:20Z

tests/test_visitors/test_ast/test_naming/test_variable_self_reassignment.py


+
+@pytest.mark.parametrize('fragment', [wrong_fragment,


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

wemake-python-styleguide/tests/test_visitors/test_ast/test_naming/test_variable_usages.py

Lines 37 to 48 in 56ce9b9

@pytest.mark.parametrize('code', [

calling_function,

called_function,

calling_method,

called_method,

accessing_attribute,

accessed_attribute,

raising_variable,

returning_variable,

awaiting_variable,

yielding_variable,

])

sobolevn · 2018-10-17T11:23:12Z

tests/test_visitors/test_ast/test_naming/test_variable_self_reassignment.py

 def test_self_variable_reassignment(
-    assert_errors, parse_ast_tree, default_options,
+    assert_errors, parse_ast_tree, fragment, default_options,


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

sobolevn · 2018-10-17T11:24:01Z

wemake_python_styleguide/violations/best_practices.py

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


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

sobolevn · 2018-10-17T11:27:03Z

wemake_python_styleguide/visitors/ast/naming.py

+        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 (


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.

sobolevn · 2018-10-17T11:27:47Z

wemake_python_styleguide/visitors/ast/naming.py

+        else:
+            target_names = [getattr(target, 'elts', None) for target
+                            in node.targets if isinstance(target, ast.Tuple)]
+            for index in range(len(target_names)):


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

sobolevn · 2018-10-17T11:35:34Z

wemake_python_styleguide/visitors/ast/naming.py

+            node_values = []
+            if isinstance(node.value, ast.Tuple):
+                node_values = [getattr(node_value, 'id', None) for node_value
+                               in node.value.elts]


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.

sobolevn · 2018-10-17T11:36:04Z

wemake_python_styleguide/visitors/ast/naming.py

+                node_values = [getattr(node_value, 'id', None) for node_value
+                               in node.value.elts]
+            if node_values in target_names or (len(target_names) !=
+                                               len(list(set(target_names)))


There's no need to cast set to list.

Roxe322 · 2018-10-17T11:48:37Z

Thank you for your detailed review, I will try to fix all problems. About line breaking - it is very hard to fit into 80 symbols line length, and because of this such ugly line breakings are in my code :)

sobolevn · 2018-10-17T11:59:37Z

@Roxe322 the general rule is: if it is hard to fit your logic into one line - you have too many logic.
Split this line into multiple variables, functions, etc.

Roxe322 · 2018-10-17T12:07:10Z

Your linter one time told me that I have too much local variables in one method, so I tried not to meet this warning again :)

coveralls · 2018-10-17T15:15:59Z

Pull Request Test Coverage Report for Build 520

26 of 26 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 490:	0.0%
Covered Lines:	1377
Relevant Lines:	1377

💛 - Coveralls

Roxe322 · 2018-10-17T15:18:54Z

@sobolevn Fixed all problems, I think :)

sobolevn

Just a single thing to fix before merging!

Awesome job!

sobolevn · 2018-10-17T15:27:25Z

wemake_python_styleguide/visitors/ast/naming.py

@@ -171,26 +173,34 @@ def visit_Assign(self, node: ast.Assign) -> None:
 class WrongVariableAssignmentVisitor(BaseNodeVisitor):
    """Finds wrong variables assignments."""

+    def create_target_names(self, target: AssignTarget) -> AssignTargetNameList:


Should start with _, since it is a protected method.

sobolevn · 2018-10-17T15:30:00Z

tests/test_visitors/test_ast/test_naming/test_variable_self_reassignment.py

+wrong_fragment_tuple_assignment = """
+x = 1
+y = 2
+x, y = x, y


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.

AlwxSin · 2018-10-17T15:35:16Z

wemake_python_styleguide/visitors/ast/naming.py

@@ -23,6 +24,8 @@
 from wemake_python_styleguide.visitors.decorators import alias

 VariableDef = Union[ast.Name, ast.Attribute, ast.ExceptHandler]
+AssignTarget = List[ast.expr]


Suggested change

AssignTarget = List[ast.expr]

AssignTargets = List[ast.expr]

AlwxSin · 2018-10-17T15:35:27Z

wemake_python_styleguide/visitors/ast/naming.py

@@ -23,6 +24,8 @@
 from wemake_python_styleguide.visitors.decorators import alias

 VariableDef = Union[ast.Name, ast.Attribute, ast.ExceptHandler]
+AssignTarget = List[ast.expr]
+AssignTargetNameList = List[Union[str, Tuple[str]]]


Suggested change

AssignTargetNameList = List[Union[str, Tuple[str]]]

AssignTargetsNameList = List[Union[str, Tuple[str]]]

Roxe322 · 2018-10-18T10:04:31Z

@sobolevn I've added test case that you requested and also fixed naming mentioned by @AlwxSin

sobolevn · 2018-10-18T10:21:31Z

@Roxe322 awesome job!

Roxe322 · 2018-10-18T10:23:46Z

Thank you! I'm glad to help you :)

forbid variables self reassignment

dff2f17

sobolevn requested changes Oct 15, 2018

View reviewed changes

sobolevn reviewed Oct 15, 2018

View reviewed changes

Roxe322 and others added 2 commits October 15, 2018 16:50

Merge branch 'master' into master

7849f2e

improved self variable reassigning recognition

f2a091e

removed redundant list casting

e6a66fb

sobolevn requested changes Oct 17, 2018

View reviewed changes

Roxe322 added 2 commits October 17, 2018 16:51

reworked self variable reassignment visitor

9d3b736

mypy error fix

f4f6dca

sobolevn requested changes Oct 17, 2018

View reviewed changes

AlwxSin reviewed Oct 17, 2018

View reviewed changes

added test case

2c9f5bd

protected create_target_names method

6a72c08

sobolevn approved these changes Oct 18, 2018

View reviewed changes

sobolevn merged commit ed3562d into wemake-services:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#212] Forbid variables self reassignment #262

[#212] Forbid variables self reassignment #262

Roxe322 commented Oct 15, 2018

sobolevn left a comment

sobolevn Oct 15, 2018

Roxe322 Oct 17, 2018

sobolevn commented Oct 15, 2018

sobolevn Oct 15, 2018

Roxe322 Oct 17, 2018

Roxe322 commented Oct 17, 2018

Roxe322 commented Oct 17, 2018

sobolevn left a comment •

edited

Loading

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018

Roxe322 commented Oct 17, 2018

sobolevn commented Oct 17, 2018

Roxe322 commented Oct 17, 2018

coveralls commented Oct 17, 2018 •

edited

Loading

Roxe322 commented Oct 17, 2018

sobolevn left a comment

sobolevn Oct 17, 2018

sobolevn Oct 17, 2018 •

edited

Loading

AlwxSin Oct 17, 2018

AlwxSin Oct 17, 2018

Roxe322 commented Oct 18, 2018

sobolevn commented Oct 18, 2018

Roxe322 commented Oct 18, 2018

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

	if node_values in target_names or (len(target_names) !=
	len(list(set(target_names)))
	):

	target_names = [getattr(target, 'elts', None) for target
	in node.targets if isinstance(target, ast.Tuple)]
	for index in range(len(target_names)):
	target_names[index] = [name.id for name in target_names[index]]

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

	AssignTargetNameList = List[Union[str, Tuple[str]]]
	AssignTargetsNameList = List[Union[str, Tuple[str]]]



		@pytest.mark.parametrize('fragment', [wrong_fragment,

[#212] Forbid variables self reassignment #262

[#212] Forbid variables self reassignment #262

Conversation

Roxe322 commented Oct 15, 2018

I have made things!

Related issues

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roxe322 commented Oct 17, 2018

Roxe322 commented Oct 17, 2018

sobolevn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roxe322 commented Oct 17, 2018

sobolevn commented Oct 17, 2018

Roxe322 commented Oct 17, 2018

coveralls commented Oct 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 520

💛 - Coveralls

Roxe322 commented Oct 17, 2018

sobolevn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolevn Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roxe322 commented Oct 18, 2018

sobolevn commented Oct 18, 2018

Roxe322 commented Oct 18, 2018

sobolevn left a comment •

edited

Loading

coveralls commented Oct 17, 2018 •

edited

Loading

sobolevn Oct 17, 2018 •

edited

Loading