You could instead write this as:

BINARY_OPERATORS = {
...
}
...
operator = BINARY_OPERATORS.get(node.op)
if operator:
    ...
    

And save yourself the getter functions and make those dicts constants.

Might as well check that this isn't None as well, e.g. if operator:

Operator should probably be lowercase since it's a module name.

You probably want to make sure that the unary operation's operand is just a variable with the same id as the lambda's operand (e.g. you shouldn't recommend replacing lambda a: not (a + 3)

will ensuring that the unary operation's operand is not an instance of BinOp suffice?

Probably not, check its type and make sure that it's the lambda variable.

I think you will false-fail on f = lambda x: x * 3.

Please add a test to ensure that you don't.

@erikwright just wondering, since we're preferring operator.mul(x, y) to f = lambda x, y: x * y, shouldn't we prefer operator.mul(x, 3) to f = lambda x: x * 3 ?

@zeedann Here is the guideline you are implementing:

use the functions from the operator module instead of lambda functions

Here's the case we are discussing:

f = lambda x: x * 3

What is the example code that you are proposing?

f = lambda x: operator.mul(x, 3)

Why is this better? It is not replacing a lambda with anything. It's replacing a bare operator with a function call.

Here is an alternative that replaces the lambda:

f = functools.partial(operator.mul, 3)

But that's a bad idea :)

I see, I had a misinterpretation of what operator.mul actually did. Thanks for clarifying!

We probably don't want to be recommending partial replacement of lambdas, e.g. lambda a, b: a * b + c shouldn't be replaced with lambda a, b: operator.mul(a, b + c)

should i be recommending lambda a, b, c; operator.mul(a, operator.add(b, c)) instead?

For common operations like multiplication, use the functions from the operator module instead of lambda functions. For example, prefer operator.mul to lambda x, y: x * y.

lambda a, b: a * b is definitely harder to read than operator.mul when used in reduce, e.g.

reduce(lambda a, b: a * b, integers)
reduce(operator.mul, integers)

But for larger expressions, I'd argue that lambda a, b, c: a * b + c is easier to read than lambda a, b, c: operator.mul(a, operator.add(b, c)), but it's debatable e.g.

c = 3
reduce(lambda a, b: a * b + c, integers)
reduce(lambda a, b: operator.mul(a, operator.add(b, c)), integers)

@erikwright @honkfestival @JasonMWhite thoughts?

Agree. operator.sum over lambda a, b: a + b, but lambda a, b: (a * b) + b over operator.sum(operator.mul(a, b), b)

good point guys, i'll recommend replacements for when lambda is used only on 2 variables with one operator

The point of using operator is to not need the lambda. Not to never see a primitive operator.

Can you also test a unary operator?
Also add a test that you don't reject lambda a, b: a * b * c

Please split this test into a bunch of smaller tests, one for each case you are covering.

As a more realistic example, can you re-write these as some_var = map(operator.not_, iterable)

As a more realistic example, can you re-write these as some_var = reduce(operator.mul, iterable)

Couldn't/shouldn't you put an equivalent check in the unary op case?

The check for a BinOp operand seems fragile to me. Is that the only possible operand, besides a variable reference? Couldn't it be another unary operation? Or a function call?

IIUC the node type you actually expect is astroid.Name. Maybe look for that explicitly.

please rename this to preferred_operator_function.

Add another test for:

binary_pass3 = reduce(lambda x, y: x + 1, [1, 2, 3])

Also for:

def binaryfnc():
    SOMETHING = 2
    ...
    binary_pass4 = reduce(lambda x, y: x * SOMETHING, [1,2,3,4])

Add a test for:

    binary_msg = reduce(lambda y, x: x * y, [1, 2, 3, 4])

I think if you verified that the operands were of the right node type you wouldn't have to worry about this case of literal operands, which I think you handle incorrectly anyway (see my requested test).

thanks for catching that, i didn't handle it correctly. I've added a check to the if statement to check a match between the left & right arguments, and the left & right operands respectively

Instead of creating this variable on every node visitation, it would be better to create this as a class constant, since you aren't modifying it anywhere that I can see.

Same here with the constants.

Minor point, but lamfun doesn't seem very Pythonic as a name. lambda_fun? lamba_fx?

IMO, these should be 4 separate tests.

Alternatively, it could be one test, but you pass the whole thing in at once and assert that there is only one failure. Could be made a bit more clear by naming it binary_fail

There are 2 Binary Op Test 3s

However you decide to address the test comments for test_binary_lambda_func, do the same for test_unary_lambda_func

Maybe make these allcaps? (since they're constants)