| Message ID | 20200922210101.4081073-32-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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 <jsnow@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 <ehabkost@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
On 9/23/20 12:35 PM, Eduardo Habkost wrote: > I believe these two lines above should be removed, as suggested > in patch 30, but let's ignore that for now. > Yup, headed there. >> - 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. The basic premise is: Why pass information you want to add to obj['features'] to a function to make that assignment, when you could just perform that assignment yourself? Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!) has to interrogate its arguments to make sure you gave it a dict when you give it a features argument. Type-wise, it's cleaner to perform this transformation when we KNOW we have an object than it is to defer to a more abstracted function and assert/downcast back to more specific types.
On Wed, Sep 23, 2020 at 05:43:45PM -0400, John Snow wrote: > On 9/23/20 12:35 PM, Eduardo Habkost wrote: > > I believe these two lines above should be removed, as suggested > > in patch 30, but let's ignore that for now. > > > > Yup, headed there. > > > > - 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. > > The basic premise is: > > Why pass information you want to add to obj['features'] to a function to > make that assignment, when you could just perform that assignment yourself? > > Otherwise, _make_tree, which accepts any arbitrary object (not just dicts!) > has to interrogate its arguments to make sure you gave it a dict when you > give it a features argument. Oh, I was not aware that obj could be not a dictionary. In this case, it makes lots of sense to move the magic outside the function. > > Type-wise, it's cleaner to perform this transformation when we KNOW we have > an object than it is to defer to a more abstracted function and > assert/downcast back to more specific types.
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) - if features: - obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features] 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)
_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 <jsnow@redhat.com> --- scripts/qapi/introspect.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)