1733 add lambda instance assignment violation #1911

AlexandrKhabarov · 2021-02-25T16:48:10Z

I have made things!

Checklist

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

AlexandrKhabarov · 2021-02-25T17:30:10Z

@sobolevn
Hello, Nikita!
Can you look at my request?

sobolevn

Thanks a lot! Great work! So many awesome features!

I have several things I want to discuss:

What do you think about cls. attribute assignment?
What do you think about class Some: a = lambda: ...?

sobolevn · 2021-02-25T22:34:11Z

tests/test_visitors/test_ast/test_classes/test_instance_assignment.py

+instance_lambda_assignment = """
+class Example(object):
+    def __init__(self):
+        self._attr = lambda: ...


We need to make this _attr thing here parametrized.
We need to check: attr, _attr, and __attr

We also need to parametrize self part.
We need to check: self, cls, mcs, other

sobolevn · 2021-02-25T22:34:59Z

wemake_python_styleguide/violations/best_practices.py

+       example.callback(1) will look like a method, but it is not a method.
+
+    Solution:
+        To forbid lambda assignment in instance attributes.


Do not assign lambda functions to attributes, create real methods.

sobolevn · 2021-02-25T22:35:37Z

wemake_python_styleguide/violations/best_practices.py

+
+    Reasoning:
+       This will be very tricky to understand for users.
+       example.callback(1) will look like a method, but it is not a method.


example.callback(1)

but it is not really a method

sobolevn · 2021-02-25T22:38:08Z

Closes #1733

sobolevn · 2021-02-25T22:39:34Z

@AlexandrKhabarov please, don't forget to use closes #x in your PRs, so our automation will link them to original issues 🙂

AlexandrKhabarov · 2021-02-26T07:41:14Z

Yeah, I think, it is a good point to check assignments to class instance too. I'll add it.

codecov · 2021-03-02T14:41:20Z

Codecov Report

Merging #1911 (1e0f252) into master (8f72647) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master     #1911    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          115       119     +4     
  Lines         6145      6258   +113     
  Branches      1375      1393    +18     
==========================================
+ Hits          6145      6258   +113

Impacted Files	Coverage Δ
...ake_python_styleguide/logic/complexity/overuses.py	`100.00% <ø> (ø)`
wemake_python_styleguide/presets/types/tree.py	`100.00% <ø> (ø)`
wemake_python_styleguide/visitors/ast/blocks.py	`100.00% <ø> (ø)`
wemake_python_styleguide/logic/tree/annotations.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/logic/tree/assignments.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/logic/tree/decorators.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/logic/tree/functions.py	`100.00% <100.00%> (ø)`
...ake_python_styleguide/violations/best_practices.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/visitors/ast/classes.py	`100.00% <100.00%> (ø)`
wemake_python_styleguide/visitors/ast/compares.py	`100.00% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f72647...8f6c965. Read the comment docs.

AlexandrKhabarov · 2021-03-02T14:42:24Z

@sobolevn
Hello, Nikita?
Can you review my request?

sobolevn

Thanks a lot, @AlexandrKhabarov!
As always, great work! 👍

sobolevn · 2021-03-02T14:47:56Z

wemake_python_styleguide/logic/tree/assignments.py

+    return flatten_nodes
+
+
+def get_assignments(node: ast.ClassDef) -> _AllAssignments:


This function is really similar to https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/logic/tree/classes.py#L76

Can you please explain, why can't we reuse it?

I wrote another one function for finding assignments only.
This one finds assignments for classes and attributes for instances and I decided to unify returned values for lambda assignment violations.

I agree that both functions are similar and I need to think about how to decreases code duplication.

sobolevn · 2021-03-02T14:48:58Z

tests/test_visitors/test_ast/test_classes/test_instance_assignment.py

+    AttributesAssignmentVisitor,
+)
+
+instance_lambda_assignment = """


We also need cls template:

""" class Example(object): @classmethod def some(cls): {0} """

…ents

AlexandrKhabarov · 2021-03-08T15:00:16Z

@sobolevn
Hello Nikita!
I decreased code duplication and write one more tests for class lamda assignment.
Can you review my request?

sobolevn

Thanks a lot! The progress is looking great! 👍

Please, rebase your PR on master, since now it has some conflicts.

sobolevn · 2021-03-10T09:25:11Z

wemake_python_styleguide/logic/tree/classes.py

+            isinstance(target, ast.Attribute) and
+            isinstance(target.ctx, ast.Store) and
+            isinstance(target.value, ast.Name) and
+            target.value.id in constants.SPECIAL_ARGUMENT_NAMES_WHITELIST


This is not correct, because constants.SPECIAL_ARGUMENT_NAMES_WHITELIST also has mcs and cls, so it can catch non-self assigns.

sobolevn · 2021-03-10T09:25:56Z

wemake_python_styleguide/visitors/ast/blocks.py

@@ -76,7 +76,7 @@ class BlockVariableVisitor(base.BaseNodeVisitor):
    )

    _scope_predicates: Tuple[_ScopePredicate, ...] = (
-        lambda node, names: predicates.is_property_setter(node),
+        lambda node, names: predicates.is_property_setter(node),  # noqa: WPS467


This should be ignored, because here we assign Tuple and not lambda.

We should test it explicitly.

sobolevn requested changes Feb 25, 2021

View reviewed changes

sobolevn linked an issue Feb 25, 2021 that may be closed by this pull request

Forbid assigning lambda to object properties #1733

Open

AlexandrKhabarov force-pushed the issue-1733 branch 3 times, most recently from 11e1d7f to f9c36c5 Compare February 26, 2021 07:27

1733 add lambda instance assignment violation

9af8176

AlexandrKhabarov force-pushed the issue-1733 branch from f9c36c5 to 9af8176 Compare February 26, 2021 07:38

AlexandrKhabarov force-pushed the issue-1733 branch 13 times, most recently from 9bbf386 to 2de3cdf Compare March 2, 2021 14:24

1733 add lambda class assignment violation

816cb36

AlexandrKhabarov force-pushed the issue-1733 branch from 2de3cdf to 816cb36 Compare March 2, 2021 14:35

sobolevn requested changes Mar 2, 2021

View reviewed changes

AlexandrKhabarov force-pushed the issue-1733 branch from f534c8c to 924525d Compare March 8, 2021 14:32

AlexandrKhabarov force-pushed the issue-1733 branch 2 times, most recently from db6c466 to 14b498c Compare March 8, 2021 14:45

1733 return only assignments and rename get_attributes to get_assignm…

8f6c965

…ents

AlexandrKhabarov force-pushed the issue-1733 branch from 14b498c to 8f6c965 Compare March 8, 2021 14:49

sobolevn requested changes Mar 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1733 add lambda instance assignment violation #1911

1733 add lambda instance assignment violation #1911

AlexandrKhabarov commented Feb 25, 2021

AlexandrKhabarov commented Feb 25, 2021

sobolevn left a comment

sobolevn Feb 25, 2021

AlexandrKhabarov Feb 26, 2021

sobolevn Feb 25, 2021

AlexandrKhabarov Feb 26, 2021

sobolevn Feb 25, 2021

AlexandrKhabarov Feb 26, 2021

sobolevn commented Feb 25, 2021

sobolevn commented Feb 25, 2021

AlexandrKhabarov commented Feb 26, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

AlexandrKhabarov commented Mar 2, 2021

sobolevn left a comment

sobolevn Mar 2, 2021 •

edited

AlexandrKhabarov Mar 2, 2021

sobolevn Mar 2, 2021

AlexandrKhabarov commented Mar 8, 2021

sobolevn left a comment

sobolevn Mar 10, 2021

sobolevn Mar 10, 2021

		return flatten_nodes


		def get_assignments(node: ast.ClassDef) -> _AllAssignments:

1733 add lambda instance assignment violation #1911

Are you sure you want to change the base?

1733 add lambda instance assignment violation #1911

Conversation

AlexandrKhabarov commented Feb 25, 2021

I have made things!

Checklist

AlexandrKhabarov commented Feb 25, 2021

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

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

sobolevn commented Feb 25, 2021

sobolevn commented Feb 25, 2021

AlexandrKhabarov commented Feb 26, 2021 • edited

codecov bot commented Mar 2, 2021 • edited

Codecov Report

AlexandrKhabarov commented Mar 2, 2021

sobolevn left a comment

Choose a reason for hiding this comment

sobolevn Mar 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandrKhabarov commented Mar 8, 2021

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

AlexandrKhabarov commented Feb 26, 2021 •

edited

codecov bot commented Mar 2, 2021 •

edited

sobolevn Mar 2, 2021 •

edited