On Tue, Sep 22, 2020 at 05:00:54PM -0400, John Snow wrote: > _make_tree doesn't know if it is receiving an object or some other type; > adding features information should arguably be performed by the caller. > > This will help us refactor _make_tree more gracefully in the next patch. > > Signed-off-by: John Snow <js...@redhat.com> > --- > scripts/qapi/introspect.py | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 41ca8afc67..e1edd0b179 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -10,7 +10,7 @@ > See the COPYING file in the top-level directory. > """ > > -from typing import (NamedTuple, Optional, Sequence) > +from typing import (NamedTuple, List, Optional, Sequence) > > from .common import ( > c_name, > @@ -20,7 +20,7 @@ > ) > from .gen import QAPISchemaMonolithicCVisitor > from .schema import (QAPISchemaArrayType, QAPISchemaBuiltinType, > - QAPISchemaType) > + QAPISchemaFeature, QAPISchemaType) > > > class Extra(NamedTuple): > @@ -31,12 +31,10 @@ class Extra(NamedTuple): > ifcond: Sequence[str] = tuple() > > > -def _make_tree(obj, ifcond, features, > +def _make_tree(obj, ifcond, > extra: Optional[Extra] = None): > comment = extra.comment if extra else None > extra = Extra(comment, ifcond)
I believe these two lines above should be removed, as suggested in patch 30, but let's ignore that for now. > - if features: > - obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features] I can't say I understand completely why moving these two lines outside _make_tree() is useful, but if it makes the cleanup work you did easier, I trust this is the right thing to do. The changes look correct. Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> > return (obj, extra) > > > @@ -169,6 +167,10 @@ def _use_type(self, typ): > return '[' + self._use_type(typ.element_type) + ']' > return self._name(typ.name) > > + @classmethod > + def _gen_features(cls, features: List[QAPISchemaFeature]): > + return [_make_tree(f.name, f.ifcond) for f in features] > + > def _gen_tree(self, name, mtype, obj, ifcond, features): > extra = None > if mtype not in ('command', 'event', 'builtin', 'array'): > @@ -179,13 +181,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features): > name = self._name(name) > obj['name'] = name > obj['meta-type'] = mtype > - self._trees.append(_make_tree(obj, ifcond, features, extra)) > + if features: > + obj['features'] = self._gen_features(features) > + self._trees.append(_make_tree(obj, ifcond, extra)) > > def _gen_member(self, member): > obj = {'name': member.name, 'type': self._use_type(member.type)} > if member.optional: > obj['default'] = None > - return _make_tree(obj, member.ifcond, member.features) > + if member.features: > + obj['features'] = self._gen_features(member.features) > + return _make_tree(obj, member.ifcond) > > def _gen_variants(self, tag_name, variants): > return {'tag': tag_name, > @@ -193,7 +199,7 @@ def _gen_variants(self, tag_name, variants): > > def _gen_variant(self, variant): > obj = {'case': variant.name, 'type': self._use_type(variant.type)} > - return _make_tree(obj, variant.ifcond, None) > + return _make_tree(obj, variant.ifcond) > > def visit_builtin_type(self, name, info, json_type): > self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None) > -- > 2.26.2 > -- Eduardo