Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions shopify_python/google_styleguide.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ class GoogleStyleGuideChecker(checkers.BaseChecker):
"For example: x = 1 if cond else 2. "
"Conditional Expressions okay to use for one-liners. "
"In other cases prefer to use a complete if statement. "),
'C6014': ('Prefer operator module function %(op)s to lambda function %(lambda_fun)s',
'lambda-func',
"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."),
}

options = (
Expand All @@ -108,6 +112,28 @@ class GoogleStyleGuideChecker(checkers.BaseChecker):
'help': 'Number of AST nodes permitted in a lambda'}),
)

UNARY_OPERATORS = {
"~": "invert",
"-": "neg",
"not": "not_",
"+": "pos"
}

BINARY_OPERATORS = {
"+": "add",
"-": "sub",
"*": "mul",
"/": "truediv",
"**": "pow",
"%": "modulo",
"<": "lt",
"<=": "le",
"==": "eq",
"!=": "ne",
">=": "ge",
">": "gt"
}

def visit_assign(self, node): # type: (astroid.Assign) -> None
self.__avoid_global_variables(node)

Expand All @@ -116,6 +142,7 @@ def visit_excepthandler(self, node): # type: (astroid.ExceptHandler) -> None

def visit_lambda(self, node): # type: (astroid.Lambda) -> None
self.__use_simple_lambdas(node)
self.__lambda_func(node)

def visit_listcomp(self, node): # type: (astroid.ListComp) -> None
self.__use_simple_list_comp(node)
Expand Down Expand Up @@ -253,3 +280,25 @@ def __use_cond_expr(self, node): # type: (astroid.If) -> None
else_body_name = node.orelse[0].targets[0].name
if if_body_name == else_body_name:
self.add_message('cond-expr', node=node)

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

if isinstance(node.body, astroid.UnaryOp):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@zeedann zeedann Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

operator = self.UNARY_OPERATORS[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:
varname = node.body.operand.name
lambda_fun = "lambda " + varname + ": " + node.body.op + " " + varname
op_fun = "operator." + operator
self.add_message('lambda-func', node=node, args={'op': op_fun, 'lambda_fun': lambda_fun})
elif isinstance(node.body, astroid.BinOp):
if shopify_python.ast.count_tree_size(node.body) == 3 and len(node.args.args) == 2:
node = node.body
operator = self.BINARY_OPERATORS[node.op]
if operator:
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
lambda_fun = "lambda " + left + ', ' + right + ": " + " ".join([left, node.op, right])
op_fun = "operator." + operator
self.add_message('lambda-func', node=node, args={'op': op_fun, 'lambda_fun': lambda_fun})
31 changes: 31 additions & 0 deletions tests/shopify_python/test_google_styleguide.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,34 @@ def test_use_cond_exprs(self):
# root_not_msg should add no messages
with self.assertNoMessages():
self.walk(root_not_msg)

def test_unary_lambda_func(self):
unary_root = astroid.builder.parse("""
def unaryfnc():
unary_pass = map(lambda x: not (x+3), [1, 2, 3, 4])
unary_fail = map(lambda x: -x, [1, 2, 3, 4])
""")

ulam_fail = unary_root.body[0].body[1].value.args[0]
unary_message = pylint.testutils.Message('lambda-func', node=ulam_fail, args={
'op': 'operator.neg',
'lambda_fun': 'lambda x: - x'
})
with self.assertAddsMessages(unary_message):
self.walk(unary_root)

def test_binary_lambda_func(self):
binary_root = astroid.builder.parse("""
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_fail = reduce(lambda x, y: x * y, [1, 2, 3, 4])
""")

binary_fail = binary_root.body[0].body[2].value.args[0]
bin_message = pylint.testutils.Message('lambda-func', node=binary_fail.body, args={
'op': 'operator.mul',
'lambda_fun': 'lambda x, y: x * y'
})
with self.assertAddsMessages(bin_message):
self.walk(binary_root)