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

[John Snow <jsnow@redhat.com>]

On 12/15/20 11:55 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> 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.
> 
> Recommend to be explict: _make_tree()'s first argument can be ...
> 
>>       It *usually* takes a dict. sometimes it doesn't.
> 
> Yes.  It takes an abstract syntax tree: dict for JSON object, list for
> JSON array, str for JSON string, bool for JSON true and false, NoneType
> for JSON none.  JSON int isn't implemented, because it doesn't occur in
> SchemaInfo.
> 
>>>>                                                       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.
> 
> Non-issue here, sub-classes are vanishingly unlikely.
> 

True. I did admit it was just simply my default. I can adjust it 
case-by-case for circumstances when ...

>> @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.
> 
> Unused cls parameters are slightly annoying, though.
> 
> I've been using @staticmethod whenever it suffices.  Makes "this is a
> function, i.e. it can't mess with the class or instances" immediately
> obvious.
> 

... you feel it provides additional clarity to do so.

--js