Skip to content

Fix 868 #1282

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

Closed
wants to merge 3 commits into from
Closed

Fix 868 #1282

wants to merge 3 commits into from

Conversation

mshafer-NI
Copy link

@mshafer-NI mshafer-NI commented May 23, 2025

The current string based search for E731 (assignment of lambda to variable) relies on a text search. This breaks down if the lambda has a comment stored on it forcing a multi-line assignment in the form of:

f = (  # some long comment that forces this to be on a new line
    lambda x: 731 * x
)

However, existing tests indicate that

f = (lambda o: o.lower()) if isinstance('a', str) else (lambda o: o)
# and
f = (
    lambda o: o.lower(),
    lambda o: o.upper(),
)

Should be considered acceptable, so merely stripping off paranthesis before evaluating would not work.

Implementation

By loading the "line" (in this example, logical_line comes in as f = (lambda x: 731 * x)) using the AST parser, we are able to check if there is an assignment of a Lambda to a Name.

This allows all existing tests to pass while also resolving the new tests for E731.

Fixes #868

@@ -46,6 +46,7 @@
700 statements
900 syntax error
"""
import ast
Copy link
Member

@asottile asottile May 23, 2025

Choose a reason for hiding this comment

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

unfortunately one of the design principles of pycodestyle is that it does not use the ast to do its linting (this allows it to be ~somewhat cross-version compatible via the tokenizer)

so while this would work -- it can't be merged :(

Copy link
Author

Choose a reason for hiding this comment

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

May I suggest documenting that in CONTRIBUTING.rst?

Copy link
Author

Choose a reason for hiding this comment

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

Would you consider a PR that attempts ast and if there's any issue, falls back to the current method?

Copy link
Member

Choose a reason for hiding this comment

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

no I would not, that's why I closed this

Copy link
Member

Choose a reason for hiding this comment

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

and it's already in the docs:

Using the ast module defeats that purpose

@asottile asottile closed this May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E731: assignment of lambda is not detected if parenthesized
2 participants