bpo-39981: Default values for AST nodes #19031
Conversation
|
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 |
|
Some initial comments, without commenting on the approach itself. @serhiy-storchaka should confirm that he is happy with this approach as he was working on some related functionality. |
Lib/ast.py
Outdated
| @@ -101,8 +102,20 @@ def _convert(node): | |||
| return _convert_signed_num(node) | |||
| return _convert(node_or_string) | |||
|
|
|||
| @lru_cache | |||
| def _field_defaults(node_type): | |||
There was a problem hiding this comment.
I don't understand this function. When we set the node_type._field_defaults we could already place None or [] in there instead of this intermediary "?" and "*".
There was a problem hiding this comment.
I dont think so, at least for lists. We are keeping _field_defaults as the same way we keep _fields, arrays of const char pointers. And then we construct a tuple from them and intern the values. I dont think it would be practical to do this with the original values.
There was a problem hiding this comment.
I don't fully understand the problem. What is wrong with having _field_defaults as a list of the defaults themselves? When constructing the _field_defaults tuple you can do this logic already and populate them with None or the empty list or whatever, no?
For instance, in the make_type function you could set the defaults in the tuple instead of interning the strings.
There was a problem hiding this comment.
Ah, that makes sense. Do you have an idea about what we should put if there is no field default, I think Ellipsis should work.
There was a problem hiding this comment.
You don't need that if you use a dictionary instead of a tuple
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @pablogsal: please review the changes made to this pull request. |
isidentical
force-pushed
the
bpo-39981
branch
2 times, most recently
from
9cdb15f to
0cbc2a8
Compare
|
@serhiy-storchaka It would be awesome to have your review / thoughts on this before alpha cut. |
|
Closing this in favor of #21417 |
https://bugs.python.org/issue39981