classification
Title: Improve ast.literal_eval test suite coverage
Type: enhancement Stage: needs patch
Components: Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: RJ722, benjamin.peterson, larry, mdk, ncoghlan, rhettinger, serhiy.storchaka
Priority: normal Keywords: easy, newcomer friendly

Created on 2013-03-20 02:34 by ncoghlan, last changed 2020-06-28 11:01 by RJ722.

Files
File name Uploaded Description Edit
issue17490_ast_literal_eval_converters.diff ncoghlan, 2013-03-21 01:44 Conversion function based extensible ast.literal_eval review
Messages (16)
msg184722 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-20 02:34
Currently, ast.literal_eval uses a long if/elif chain to implement the limited parsing of the AST. This issue proposes changing the implementation to make it easy to extend to cover some of the implementation details of PEP 436 (Argument Clinic).
msg184733 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-20 04:17
Interesting patch. :)
msg184736 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-20 04:24
Heh, that's what I get for wanting to go get dinner. I'll regenerate it in
the morning.
msg184826 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 01:44
Updated patch attached. The complexity grew a bit, as it turned out to be useful for the converter functions to be able to tell the difference between top level conversions and recursive invocations. I also realised that:

- the complex number conversion currently allows arbitrarily deep nesting of addition and subtraction
- handling this properly made an exception based "Failed to convert" API far more sensible

The docstrings are updated, but ast.convert_node is tested only indirectly through the ast.literal_eval tests and the main docs haven't been updated at all.

At this point, I'm most interested in feedback from Larry as to whether or not this is helpful to him in implementing argument clinic.
msg184827 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 01:56
Now that I've actually implemented it, I'm fairly sure it's not a good idea. It might be better if we remove the recursive behaviour.

As it is, Larry's fairly happy with what he is already doing (calling ast.parse and walking the tree) and I think that's actually simpler than using this API would be.
msg184839 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2013-03-21 03:19
FWIW, I like the current version of the code and find it easily to understand what it does and how it does it.   The new code is longer and full of indirections.   I question whether it is a real improvement.
msg184842 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-03-21 04:45
For what it's worth, here's what we're doing.  The new Argument Clinic "parameter" line looks a great deal like the middle bit of a Python function declaration.  So much so that we can do this:

    ast_string = "def x({}): pass".format(parameter_line)
    root = ast.parse(ast_string)

We then walk the node tree and pull out what we need.  We do a similar trick to parse the return converter, which comes from another line.  ("def x() {}: pass")

The "converter function" can look like a literal string, an id, or a function call.  When we see the function call form, we pull out each keyword and its value, then pass the value node to ast.literal_eval().  Easy peasy and Bob's your uncle.
msg184873 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-03-21 15:02
I don't think we should change the implementation, but I also discovered several holes in the test suite coverage while refactoring it. We should pursue some of the test suite changes, including switching them over to be data driven and take advantage of the new subtest support.
msg282113 - (view) Author: Julien Palard (mdk) * (Python committer) Date: 2016-11-30 23:18
> I also discovered several holes in the test suite coverage while refactoring it.

Are they all tested in the current diff?
msg282132 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-01 03:51
I don't know if the suggested test suite diff gives full coverage, but unless other improvements have been made to those tests in the meantime, it should give more coverage than we have right now.
msg282160 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-01 12:06
I agreed with Raymond, the current version of the code looks clearer.
msg282164 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-01 12:21
Right, the only reason this is still open is because I thought the extra test cases might be interesting in their own right (just not interesting enough to sit down and apply).
msg330200 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-21 13:06
Nick, do you mind to create a PR for new tests?

Some additional tests were added since this issue was created.
msg372500 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2020-06-28 07:15
Belatedly removing the issue assignment here, as I'm not actively working on this.

I've also marked this as an easy newcomer friendly task, as all that's involved is taking the `test_ast.py` changes from https://bugs.python.org/file29520/issue17490_ast_literal_eval_converters.diff and turning them into a test suite PR on GitHub.
msg372507 - (view) Author: Rahul Jha (RJ722) * Date: 2020-06-28 09:35
Nick, hello! I'd like to take it onwards from here.
msg372512 - (view) Author: Rahul Jha (RJ722) * Date: 2020-06-28 11:01
Some of the test cases from Nick's patch are not passing on master:

    ast.literal_eval('')  # raises SyntaxError; expected: ValueError
    ast.literal_eval('6j--3')  # expected: 3+6j
    ast.literal_eval('(2j+4j)+(1+2)')  # expected: 3+6j
    ast.literal_eval('(2j+4j)-(1+2)')  # expected: -3+6j

I'm assuming that new changes in ast.py do not allow for these. Can anyone confirm that this is indeed the case, and not a bug with literal_eval?
History
Date User Action Args
2020-06-28 11:01:34RJ722setmessages: + msg372512
2020-06-28 09:35:18RJ722setnosy: + RJ722
messages: + msg372507
2020-06-28 07:15:41ncoghlansetkeywords: + easy, newcomer friendly, - patch
assignee: ncoghlan ->
messages: + msg372500

stage: patch review -> needs patch
2018-11-22 13:00:10ncoghlansetassignee: ncoghlan
2018-11-21 13:06:36serhiy.storchakasetmessages: + msg330200
2016-12-01 12:21:42ncoghlansetmessages: + msg282164
2016-12-01 12:06:04serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg282160
2016-12-01 03:51:36ncoghlansetmessages: + msg282132
2016-11-30 23:18:41mdksetnosy: + mdk
messages: + msg282113
2013-03-21 15:02:10ncoghlansetmessages: + msg184873
title: Use converter functions to implement ast.literal_eval -> Improve ast.literal_eval test suite coverage
2013-03-21 04:45:40larrysetmessages: + msg184842
2013-03-21 03:19:20rhettingersetnosy: + rhettinger
messages: + msg184839
2013-03-21 01:56:31ncoghlansetmessages: + msg184827
2013-03-21 01:44:26ncoghlansetfiles: - ast_literal_eval_converter_functions.diff
2013-03-21 01:44:04ncoghlansetfiles: + issue17490_ast_literal_eval_converters.diff

messages: + msg184826
2013-03-20 04:24:51ncoghlansetmessages: + msg184736
2013-03-20 04:17:43benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg184733
2013-03-20 02:35:01ncoghlansetnosy: + larry
2013-03-20 02:34:35ncoghlancreate