1733 add lambda instance assignment violation by AlexandrKhabarov · Pull Request #1911 · wemake-services/wemake-python-styleguide

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

+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

+       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

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

+    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

+    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

+            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


    _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

Closed

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

sobolevn mentioned this pull request Dec 16, 2024

Add WPS617, which forbits to assign lambda to attrs #3172

Merged

sobolevn closed this in #3172 Dec 16, 2024

		return flatten_nodes


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

Uh oh!

Conversation

AlexandrKhabarov commented Feb 25, 2021

I have made things!

Checklist

Uh oh!

AlexandrKhabarov commented Feb 25, 2021

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Feb 25, 2021

Uh oh!

sobolevn commented Feb 25, 2021

Uh oh!

AlexandrKhabarov commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AlexandrKhabarov commented Mar 2, 2021

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

sobolevn Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexandrKhabarov commented Mar 8, 2021

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexandrKhabarov commented Feb 26, 2021 •

edited

Loading

codecov Bot commented Mar 2, 2021 •

edited

Loading

sobolevn Mar 2, 2021 •

edited

Loading