Skip to content

Conversation

@isidentical
Copy link
Member

@isidentical isidentical commented Dec 19, 2019

Based on changes from #340.

https://bugs.python.org/issue32888

@isidentical isidentical changed the title bpo-32888: improve wording for ast.literal_eval error messages bpo-32888: enhance ast.literal_eval error messagess with context information May 13, 2020
def _convert_num(node):
def _raise_malformed_node(node, context = None):
message = "malformed node or string"
if context is not None and isinstance(node, AST):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants