sigopt / sigoptlite

Optimize with SigOpt with this standalone SigOpt client driver.
Apache License 2.0
8 stars 1 forks source link

Use Assignment Expression (Walrus) In Conditional #42

Closed pixeebot[bot] closed 8 months ago

pixeebot[bot] commented 8 months ago

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 * [https://docs.python.org/3/whatsnew/3.8.html#assignment-expressions](https://docs.python.org/3/whatsnew/3.8.html#assignment-expressions)

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

tjs-intel commented 8 months ago

no thanks 👎

dwanderson-intel commented 8 months ago

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

drdavella commented 8 months ago

@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: https://github.com/pixee/codemodder-python/issues/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 commented 8 months ago

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 commented 8 months ago

@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.