Changed the way how parameters are being built #314
Conversation
PCManticore
force-pushed
the
arguments-215
branch
from
83df180 to
abfb8cb
Compare
|
I would use "Parameter" as the name for the new node rather than "Param." I don't like shortening names into non-words unless it's necessary, and I don't think it is here. For instance, inspect/funcsigs calls their parameter object "Parameter." For the implementation of Empty, I was planning to have |
|
Thanks, these make sense! I'll update the PR. |
| defaults = [] | ||
| annotations = [] | ||
| def _extract_args(parameters, parent): | ||
| """Generate an iterator of params from Python parameters.""" |
There was a problem hiding this comment.
Since we're now going to have two objects named Parameter (the astroid node and the inspect/funcsigs.Parameter), I'd rewrite this to say something like, "Generates an iterator of Parameter nodes from an iterator of inspect/funcsigs.Parameter objects."
|
I think that our handling of positional-only and keyword-only arguments ought to be consistent: there should be another field in Arguments nodes to hold positional-only arguments like |
| @@ -113,6 +114,53 @@ def _get_context(node): | |||
| return CONTEXTS.get(type(node.ctx), astroid.Load) | |||
|
|
|||
|
|
|||
| class ParameterVisitor(object): | |||
There was a problem hiding this comment.
This is a quibble but this could probably be a subclass of ast.NodeVisitor. In the future, I'd like to reorganize the rebuilder to make it so that the main visitor contains only methods corresponding to the standard library AST's nodes, and move all other nodes to independent functions or something else here. We might also consider if using the ast.NodeVisitor is appropriate for the main visitor: it might offer better forward compatibility when the CPython developers change something in the standard library AST. On the other hand, it wouldn't work with turning the visitor methods into generators, which I proposed to prevent it from overflowing the call stack and which might also speed it up. If we're going to end up moving to another parser like 2to3 or Dropbox's altered version of the CPython parser, all this is moot, though.
The old way consisted in having the parameter names, their defaults and their annotations separated in different components of the Arguments node. We introduced a new Param node, which holds the name of a parameter, its default value and its annotation. If any of the last two values are missing, then that slot will be filled with a new node kind, Empty, which is used for specifying the lack of something (None could have been used instead, but that means having non-AST nodes in the Arguments node). We're also having support for positional only arguments, for the moment only in raw_building. Close #215
PCManticore
force-pushed
the
arguments-215
branch
from
abfb8cb to
6d25296
Compare
|
Thanks for the review. Here's the updated version. I dropped the positional_only attribute from the Parameter node and went with separate lists of arguments, but I also included a new attribute in the Arguments node, positional_and_keyword, which holds both positional only and positional_or_keyword arguments. That's because when processing the arguments we're interested in both, while trying to separate their processing from kwonlyargs, so whenever .args.args was used, it got replaced with positional_and_keyword list. Decided not to subclass ParameterVisitor from ast.NodeVisitor, it's not a big deal for now and we're planning to change it anyway. |
| @@ -46,8 +47,18 @@ def _extract_expressions(node): | |||
| child = getattr(node.parent, name) | |||
| if isinstance(child, (list, tuple)): | |||
| for idx, compound_child in enumerate(child): | |||
| if compound_child is node: | |||
|
|
|||
| # Can't find a cleaner way to do this. | |||
There was a problem hiding this comment.
This comment could be expanded to clarify what this code is doing. (I'm not clear on its purpose.)
|
I added a few final comments, otherwise looks good to me. |
Changed the way how parameters are being built
The old way consisted in having the parameter names, their defaults and their annotations separated in different components of the Arguments node. We introduced a new Param node, which holds
the name of a parameter, its default value and its annotation. If any of the last two values are missing, then that slot will be filled with a new node kind, Empty, which is used for specifying the
lack of something (None could have been used instead, but that means having non-AST nodes in the Arguments node). We're also having support for positional only arguments, for the moment only in raw_building, but we can add later on for extension modules and other builtins. I decided to add a flag to the Param nodes which describes it as being positional only or not, because introducing another object to the Arguments node would mean to iterate over both lists (.args and .positional_only_args) when wanting to process all the parameters of a function, so this seems way easier to me to manage.
Close #215
@ceridwen Please do a review when you'll get some time.