Re: [PATCH v2 06/11] qapi/introspect.py: add _gen_features helper

[John Snow]

On 11/16/20 3:47 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > _make_tree might receive a dict or some other type.
>
> Are you talking about @obj?

Yes. It *usually* takes a dict. sometimes it doesn't.

> >                                                      Adding features
> > information should arguably be performed by the caller at such a time
> > when we know the type of the object and don't have to re-interrogate it.
>
> Fair enough.  There are just two such callers anyway.
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/introspect.py | 19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 803288a64e7..16282f2634b 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -76,16 +76,12 @@
> >   
> >   
> >   def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> > -               features: List[QAPISchemaFeature],
> >                  extra: Optional[Annotations] = None
> >                  ) -> TreeValue:
> >       if extra is None:
> >           extra = {}
> >       if ifcond:
> >           extra['if'] = ifcond
> > -    if features:
> > -        assert isinstance(obj, dict)
> > -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> >       if extra:
> >           return (obj, extra)
> >       return obj
> > @@ -221,6 +217,11 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> >               return '[' + self._use_type(typ.element_type) + ']'
> >           return self._name(typ.name)
> >   
> > +    @classmethod
> > +    def _gen_features(cls,
> > +                      features: List[QAPISchemaFeature]) -> List[TreeValue]:
> > +        return [_make_tree(f.name, f.ifcond) for f in features]
> > +
>
> Ignorant question: when to use @classmethod, and when to use
> @staticmethod?

Matter of taste. My preference is to just always use @classmethod, 
because they can be extended or referenced by subclasses.

@staticmethod does not take a class argument, @classmethod does. Static 
methods therefore cannot address any other classmethods, but a 
classmethod can.

I just always reach for classmethod by default.

> >       def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> >                     ifcond: List[str],
> >                     features: Optional[List[QAPISchemaFeature]]) -> None:
> > @@ -233,7 +234,9 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> >               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: QAPISchemaObjectTypeMember) -> TreeValue:
>
> No change when not features.  Else, you change
>
>      obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>
> to
>
>      obj['features'] = [_make_tree(f.name, f.ifcond) for f in features]

Yep. I am consolidating the logic for (node, annotation) in so doing.

> where
>
>      _make_tree(f.name, f.ifcond)
>      = (f.name, {'if': f.ifcond})       if f.ifcond
>      = f.name                           else
>
> Works, and feels less lazy.  However, the commit message did not prepare
> me for this.  If you split this off into its own patch, you can describe
> it properly.

OK.

> > @@ -243,7 +246,9 @@ def _gen_member(self,
> >           }
> >           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: str,
> >                         variants: List[QAPISchemaVariant]) -> _DObject:
> > @@ -255,7 +260,7 @@ def _gen_variant(self, variant: QAPISchemaVariant) -> 
> > TreeValue:
> >               '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: str, info: Optional[QAPISourceInfo],
> >                              json_type: str) -> None: