prefer operator fncs to lambda fncs by zeedann · Pull Request #57 · Shopify/shopify_python

zeedann · 2017-04-10T20:52:17Z

Part of a rule on #36

simple-lambdas has been implemented.
This PR is for preference of Operator Fcns over Lambda Fcns (https://google.github.io/styleguide/pyguide.html?showone=Lambda_Functions#Lambda_Functions)

cfournie · 2017-04-10T21:14:27Z

shopify_python/google_styleguide.py

+     #  if shopify_python.ast.count_tree_size(node) == 1:
+      #      print node.name
+        if shopify_python.ast.count_tree_size(node) == 3:
+            operator = OperatorUtils().get_bin_op(node.op)


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.

cfournie

.

cfournie · 2017-04-11T12:58:33Z

shopify_python/google_styleguide.py

+                "-": "neg",
+                "+": "pos"
+            }
+            operator = unary_operators.get(node.body.op)


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

cfournie · 2017-04-11T13:01:33Z

shopify_python/google_styleguide.py

+                left = node.left.name
+                right = node.right.name
+                lamfun = "lambda " + left + ', ' + right + ": " + " ".join([left, node.op, right])
+                opfun = "Operator." + operator


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

cfournie · 2017-04-11T13:05:25Z

shopify_python/google_styleguide.py

+    def __lambda_func(self, node):  # type: (astroid.Lambda) -> None
+        """Prefer Operator Function to Lambda Functions"""
+
+        if isinstance(node.body, astroid.UnaryOp):


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!

cfournie · 2017-04-11T13:08:15Z

shopify_python/google_styleguide.py

+                opfun = "Operator." + operator
+                self.add_message('lambda-func', node=node, args={'op': opfun, 'lamfun': lamfun})
+            return
+        elif shopify_python.ast.count_tree_size(node) > 3:


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.

zeedann · 2017-04-11T19:22:16Z

UPDATE: changes I've made from the first iteration

For the unary case, ensured that the operand is not a BinOp, and that it is the lambda variable.
Removed the recursive code, to allow this to catch simpler lambda cases ( < 3 variables, 1 operation)

cfournie · 2017-04-11T19:32:48Z

tests/shopify_python/test_google_styleguide.py

        with self.assertNoMessages():
            self.walk(root_not_msg)
+
+    def test_lambda_func(self):


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.

cfournie · 2017-04-12T13:21:48Z

tests/shopify_python/test_google_styleguide.py

+        unary_root = astroid.builder.parse("""
+        def unaryfnc():
+            unary_pass = lambda x: not (x+3)
+            unary_msg = lambda x: -x


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

cfournie · 2017-04-12T13:22:16Z

tests/shopify_python/test_google_styleguide.py

+        def binaryfnc():
+            binary_pass = lambda x, y, z: x * y + z
+            binary_pass2 = lambda x: x + 3
+            binary_msg = lambda x, y: x * y


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

zeedann · 2017-04-12T18:09:29Z

@erikwright @JasonMWhite any final comments before I merge this? :D

erikwright · 2017-04-12T18:20:01Z

shopify_python/google_styleguide.py

+                opfun = "operator." + operator
+                self.add_message('lambda-func', node=node, args={'op': opfun, 'lamfun': lamfun})
+        elif isinstance(node.body, astroid.BinOp):
+            if shopify_python.ast.count_tree_size(node.body) == 3 and len(node.args.args) == 2:


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

erikwright · 2017-04-12T18:29:19Z

shopify_python/google_styleguide.py

+            }
+            operator = unary_operators.get(node.body.op)
+            argname = node.args.args[0].name
+            if operator and not isinstance(node.body.operand, astroid.BinOp) and argname is node.body.operand.name:


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.

erikwright · 2017-04-12T18:29:52Z

shopify_python/google_styleguide.py

+                "not": "not_",
+                "+": "pos"
+            }
+            operator = unary_operators.get(node.body.op)


please rename this to preferred_operator_function.

erikwright · 2017-04-12T18:32:29Z

tests/shopify_python/test_google_styleguide.py

+        def binaryfnc():
+            binary_pass = reduce(lambda x, y, z: x * y + z, [1, 2, 3])
+            binary_pass2 = map(lambda x: x + 3, [1, 2, 3, 4])
+            binary_msg = reduce(lambda x, y: x * y, [1, 2, 3, 4])


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])

erikwright · 2017-04-12T18:36:22Z

shopify_python/google_styleguide.py

+                }
+                operator = binary_operators.get(node.op)
+                if operator:
+                    left = str(node.left.value) if node.left.name == 'int' else node.left.name


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

zeedann · 2017-04-13T16:46:44Z

approval to merge? :D @erikwright @JasonMWhite

JasonMWhite · 2017-04-17T15:05:45Z

shopify_python/google_styleguide.py

+
+        if isinstance(node.body, astroid.UnaryOp):
+            if shopify_python.ast.count_tree_size(node.body) == 2 and len(node.args.args) == 1:
+                unary_operators = {


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.

JasonMWhite · 2017-04-17T15:08:00Z

shopify_python/google_styleguide.py

+                left_arg = node.args.args[0].name
+                right_arg = node.args.args[1].name
+                node = node.body
+                binary_operators = {


Same here with the constants.

JasonMWhite · 2017-04-17T15:09:09Z

shopify_python/google_styleguide.py

+                if preferred_operator_function and left_arg is node.left.name and right_arg is node.right.name:
+                    left = str(node.left.value) if node.left.name == 'int' else node.left.name
+                    right = str(node.right.value) if node.right.name == 'int' else node.right.name
+                    lamfun = "lambda " + left + ', ' + right + ": " + " ".join([left, node.op, right])


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

JasonMWhite · 2017-04-17T15:10:11Z

tests/shopify_python/test_google_styleguide.py

+            binary_msg = reduce(lambda x, y: x * y, [1, 2, 3, 4])
+        """)
+
+        # Binary Op Test 1: This will not trigger a message


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

JasonMWhite · 2017-04-17T15:10:27Z

tests/shopify_python/test_google_styleguide.py

+        with self.assertNoMessages():
+            self.walk(binary_pass2)
+
+        # Binary Op Test 3: This will trigger a message


There are 2 Binary Op Test 3s

JasonMWhite · 2017-04-17T15:13:03Z

tests/shopify_python/test_google_styleguide.py

+            'op': 'operator.neg',
+            'lamfun': 'lambda x: - x'
+            })
+        with self.assertAddsMessages(unary_message):


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

cfournie · 2017-04-17T18:00:07Z

shopify_python/google_styleguide.py

+        "+": "pos"
+    }
+
+    binary_operators = {


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

zeedann · 2017-04-17T18:44:59Z

I'm failing some tests that are unrelated to my PR.
I've tried updating/rebasing off master and use the make ** commands which succeed locally but fail on travis.
Here is an example: https://travis-ci.org/Shopify/shopify_python/jobs/222856408#L439
Officially stumped.
Any suggestions? :\

JasonMWhite · 2017-04-17T19:27:47Z

Can you reproduce these errors locally? Try clearing out your venv, and reinstalling, maybe a dependency got bumped?

zeedann · 2017-04-17T19:53:40Z

can't reproduce these errors locally, everything looks fine locally

cfournie · 2017-04-17T20:01:59Z

Probably not your fault, I just re-ran master and it failed, looks like they removed that function at some point from pylint (by no longer inheriting from the unit test class). We can probably downgrade pylint temporarily to fix.

upa failed new

zeedann · 2017-04-17T21:07:03Z

Green has never looked so good 💚

Thanks @cfournie for the pylint update,

UPDATES:

moved dictionary of operators as constants of the class.
shortened tests to by removing all cases of assertNoMessages and assert that only one failure message will be added

zeedann · 2017-04-20T21:12:30Z

Any last comments/thoughts before I merge this in the morning? :D

cfournie suggested changes Apr 10, 2017

View reviewed changes

zeedann force-pushed the lambda branch from 2f09877 to 8bfdf88 Compare April 11, 2017 03:06

cfournie suggested changes Apr 11, 2017

View reviewed changes

zeedann force-pushed the lambda branch from c038a32 to bf3993a Compare April 11, 2017 19:09

cfournie reviewed Apr 11, 2017

View reviewed changes

zeedann force-pushed the lambda branch from a0256aa to 2c79ea4 Compare April 12, 2017 02:16

cfournie suggested changes Apr 12, 2017

View reviewed changes

cfournie approved these changes Apr 12, 2017

View reviewed changes

erikwright reviewed Apr 12, 2017

View reviewed changes

zeedann force-pushed the lambda branch from 931aa17 to ad73b17 Compare April 12, 2017 19:22

JasonMWhite reviewed Apr 17, 2017

View reviewed changes

zeedann force-pushed the lambda branch 2 times, most recently from 75e1dd1 to 6445c02 Compare April 17, 2017 17:38

cfournie reviewed Apr 17, 2017

View reviewed changes

zeedann force-pushed the lambda branch from a9eac46 to 2bd73c7 Compare April 17, 2017 18:20

zeedann force-pushed the lambda branch from fcc3f4c to 80146fa Compare April 17, 2017 19:39

zeedann force-pushed the lambda branch from 80146fa to 5da34dd Compare April 17, 2017 19:47

updates

fa13bb5

upa failed new

zeedann force-pushed the lambda branch from 42de6f1 to fa13bb5 Compare April 17, 2017 21:03

JasonMWhite approved these changes Apr 20, 2017

View reviewed changes

cfournie approved these changes Apr 21, 2017

View reviewed changes

zeedann merged commit d8dac26 into master Apr 21, 2017

cfournie deleted the lambda branch April 21, 2017 14:08

JasonMWhite mentioned this pull request May 30, 2017

bump to 0.4.0 #71

Merged

Conversation

zeedann commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfournie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeedann Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeedann commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeedann commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikwright Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeedann Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeedann commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zeedann commented Apr 10, 2017 •

edited

Loading

zeedann Apr 11, 2017 •

edited

Loading

erikwright Apr 12, 2017 •

edited

Loading

zeedann Apr 12, 2017 •

edited

Loading

zeedann commented Apr 17, 2017 •

edited

Loading