bpo-32888: Improve exception message in ast.literal_eval by Rosuav · Pull Request #340 · python/cpython
This looks good, do you mind opening a bugs.python.org ticket and adding a news entry?
Rosuav
changed the title
Improve exception message in ast.literal_eval
bpo-32888: Improve exception message in ast.literal_eval
When a non-literal is given to literal_eval, attempt to be more helpful with the message, rather than calling it 'malformed'.
Rebased onto current master, which includes shifting where the actual change happens. NEWS entry added, issue number added.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a special test in test_ast.
| return node.n | ||
| elif isinstance(node, AST): | ||
| raise ValueError('%s not allowed in literal' % type(node).__name__) | ||
| raise ValueError('malformed node or string: ' + repr(node)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what cases this exception is raised? Is it covered by tests?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The easiest way to trigger it is to use something that isn't permitted in literal_eval, but is valid syntax. There's already one such test - proving that f-strings aren't literal_evallable - and I guess there could be more, but since this is a whitelist, the error is basically "anything else, raise". The only difference in this patch is that the message is different for "syntactically valid but not acceptable by literal_eval" and "syntactically invalid".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant what input triggers the "malformed node or string" error now?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling literal_eval with an AST node that has non-AST children in it. I don't think it's possible (post-patch) to trigger this error by calling literal_eval with a string, but I can't say for sure, so I don't want to say just "malformed node".
| @@ -0,0 +1,2 @@ | |||
| Improve wording of error messages from ast.literal_eval, distinguishing | |||
| valid-but-unacceptable nodes from those that are actually malformed. | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "Patch by..."
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be done by amending the current commit, or adding an additional commit?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better for review to add a new commit. In any case all commits are squashed before merging into the repository.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, just one suggestion on the test implementation.
| ast.BinOp(ast.Num(1), ast.Add(), 'oops'): "malformed node or string: 'oops'", | ||
| } | ||
| for test, message in tests.items(): | ||
| with self.assertRaises(ValueError) as cm: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
@Rosuav, please address the last change request by @briancurtin and also rebase to fix the conflict. Thanks!
BPO cites another couple of issues, one of which is still open, as dependencies. And it's not really a high priority - it's nothing more than a nice-to-have. There are a lot of barriers to getting patches accepted and it's often not worth the effort unless you REALLY care about the patch.
(Note that you can rebase or do a regular merge, which is often easier! All PRs are squash merged in the end @csabella)
@serhiy-storchaka Should this wait for bpo-32893 per your last message on the ticket or could it be merged soon? (when conflicts are resolved + suggestion from Brian)
A different approach using ast.dump(): PR #16620.
No, I'm not. It was a nice-to-have but between the pushback (no core dev seemed to want it to happen) and the constant need to fix merge conflicts meant that I abandoned this proposal ages ago. If someone else wants to champion this fix, then sure, but I don't have the energy to push everything, and this one wasn't worth it.
Closing this pull request as it is now GH-17662.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters