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

[John Snow <jsnow@redhat.com>]

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: