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

Use Assignment Expression (Walrus) In Conditional #42

Closed

Conversation

pixeebot[bot]
Copy link
Contributor

@pixeebot pixeebot bot commented Feb 20, 2024

This codemod updates places where two separate statements involving an assignment and conditional can be replaced with a single Assignment Expression (commonly known as the walrus operator).

Many developers use this operator in new code that they write but don't have the time to find and update every place in existing code. So we do it for you! We believe this leads to more concise and readable code.

The changes from this codemod look like this:

- x = foo()
- if x is not None:
+ if (x := foo()) is not None:
      print(x)
More reading

Powered by: pixeebot (codemod ID: pixee:python/use-walrus-if)

@tjs-intel
Copy link
Contributor

no thanks 👎

@dwanderson-intel
Copy link
Contributor

I merged some of these changes in other repos, usually just 1 or 2 lines. We can revert if we feel really strong against (still working on config settings), but I'd say if we only use the variable inside the if-block, I'm fine with it; outside the block then it's probably clearer to have it separate

@dwanderson-intel dwanderson-intel deleted the pixeebot/drip-2024-02-20-pixee-python/use-walrus-if branch February 20, 2024 21:49
@drdavella
Copy link

@tjs-intel @dwanderson-intel I'm one of the maintainers of @pixeebot. We appreciate the feedback!

We've actually got an open issue to remove unused assignments when applying this codemod: pixee/codemodder-python#264

Just curious but if your pylint job had passed would you have accepted this change? Or is it just too opinionated overall?

@tjs-intel
Copy link
Contributor

tjs-intel commented Feb 20, 2024

Hi @drdavella,

I am not a fan of the walrus operator, I think it adds unnecessary cognitive overhead.

If the variables are no longer used after in-lining the assignment then I might accept a change to remove the once-used variables. But once-used variables can sometimes be helpful if descriptive variable names are used, and can help to avoid trivial comments.

I could also be convinced of the walrus operator being used in places where the truthy-ness of the variable is the only thing being checked in the condition, ex.

if options := get_list_of_options():
  # do something with options

This type of guard condition is still very readable.

@drdavella
Copy link

@tjs-intel that's great feedback, thank you!

In general our focus is on remediating security issues although we have also implemented some more general code-quality fixes that tend to be a little bit more opinionated. This one in particular seems to elicit strong opinions in both directions so I appreciate the additional data point.

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.

3 participants