GeezerVison

Why is it necessary to check here that node is an AST and not just that context is not None?

I wonder if 'context' is overloaded. Maybe it's better not to add it here to the string passed in, and let the context arg be complete, like "literal expression" or "unary expression arg".

Why is it necessary to check here that node is an AST and not just that context is not None?

For malformed nodes (manually constructed), it might not be an AST type, example (already presented in tests);

            ast.BinOp(
                ast.Constant(1), ast.Add(), "oops"
            ): "malformed node or string: 'oops'",

I wonder if 'context' is overloaded. Maybe it's better not to add it here to the string passed in, and let the context arg be complete, like "literal expression" or "unary expression arg".

I am not a native speaker, but from the point of messages, it satisfies the expectations and gives the precise location. (+a => ValueError: malformed node or string in binary operation context: <ast.Name object at 0x7f02f058d5f0>). What would you prefer instead of the message above? By the way thanks for your bump on this old PR! I completely forgot it existed since there was no activity :)

• I see about the first point.
• On the wording - would it work to simply have "malformed binary operation" in this case?
• My pleasure 👍