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

Ensure that FalsyConstantCompareViolation counts NamedExpr #1201

Closed
sobolevn opened this issue Feb 28, 2020 · 5 comments · Fixed by #1253
Closed

Ensure that FalsyConstantCompareViolation counts NamedExpr #1201

sobolevn opened this issue Feb 28, 2020 · 5 comments · Fixed by #1253
Assignees
Labels
feature New feature or request help wanted Extra attention is needed level:starter Good for newcomers pr-merged

Comments

@sobolevn
Copy link
Member

sobolevn commented Feb 28, 2020

Currently we don't handle NamedExpr in this violation.

One can write this code:

if (x := []) == other:
    ...

And it won't be handled properly. Here's how direct values are compared:

  1:4      WPS520 Found compare with falsy constant
  if [] == other:
     ^

File to fix:

def visit_Compare(self, node: ast.Compare) -> None:

We also need to test this violation. Make sure that you use pytest.skipif(PY38, ...) because this syntax is only valid starting from python3.8+

Related #1137

@sobolevn sobolevn added this to the Version 0.14 aka python3.8 milestone Feb 28, 2020
@sobolevn sobolevn added feature New feature or request good first issue Entrypoint to the project help wanted Extra attention is needed level:starter Good for newcomers and removed good first issue Entrypoint to the project labels Feb 28, 2020
@edytagarbarz
Copy link
Contributor

Let me fix this one

@sobolevn
Copy link
Member Author

sobolevn commented Mar 9, 2020

Sure! Thank you, @edytagarbarz! Make sure to read our contributing guides

I am here to help: ask anything you need.

@sobolevn
Copy link
Member Author

@edytagarbarz friendly ping to make sure you don't have any problems with the task. But, if there's something I can help you with - feel free to ask! 👍

@sobolevn
Copy link
Member Author

By the way, this commit does pretty much the same thing as you need: 4fa9f41 Check it out!

@edytagarbarz
Copy link
Contributor

Thank you @sobolevn! Configuration took me a while but now no problems at all :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Extra attention is needed level:starter Good for newcomers pr-merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants