Re: [PATCH v2 30/38] qapi/introspect.py: Add a typed 'extra' structure

[John Snow <jsnow@redhat.com>]

On 9/23/20 12:13 PM, Eduardo Habkost wrote:
> On Tue, Sep 22, 2020 at 05:00:53PM -0400, John Snow wrote:
>> Typing arbitrarily shaped dicts with mypy is difficult prior to Python
>> 3.8; using explicit structures is nicer.
>>
>> Since we always define an Extra type now, the return type of _make_tree
>> simplifies and always returns the tuple.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 31 +++++++++++++++++++------------
>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>
> 
> Here I'm confused by both the original code and the new code.
> 
> I will try to review as a refactoring of existing code, but I'll
> have suggestions for follow ups:
> 
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index b036fcf9ce..41ca8afc67 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -10,6 +10,8 @@
>>   See the COPYING file in the top-level directory.
>>   """
>>   
>> +from typing import (NamedTuple, Optional, Sequence)
>> +
>>   from .common import (
>>       c_name,
>>       gen_endif,
>> @@ -21,16 +23,21 @@
>>                        QAPISchemaType)
>>   
>>   
>> -def _make_tree(obj, ifcond, features, extra=None):
>> -    if extra is None:
>> -        extra = {}
>> -    if ifcond:
>> -        extra['if'] = ifcond
>> +class Extra(NamedTuple):
>> +    """
>> +    Extra contains data that isn't intended for output by introspection.
>> +    """
>> +    comment: Optional[str] = None
>> +    ifcond: Sequence[str] = tuple()
>> +
>> +
>> +def _make_tree(obj, ifcond, features,
>> +               extra: Optional[Extra] = None):
>> +    comment = extra.comment if extra else None
>> +    extra = Extra(comment, ifcond)
> 
> Here we have one big difference: now `extra` is being recreated,
> and all fields except `extra.comment` are being ignored.  On the
> original version, all fields in `extra` were being kept.  This
> makes the existence of the `extra` argument pointless.
> 

Yup, oops.

> If you are going through the trouble of changing the type of the
> 4rd argument to _make_tree(), this seems more obvious:
> 

Yes, agree. I came up with something similar after talking to you this 
morning.

>    diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>    index 41ca8afc672..c62af94c9ad 100644
>    --- a/scripts/qapi/introspect.py
>    +++ b/scripts/qapi/introspect.py
>    @@ -32,8 +32,7 @@ class Extra(NamedTuple):
>     
>     
>     def _make_tree(obj, ifcond, features,
>    -               extra: Optional[Extra] = None):
>    -    comment = extra.comment if extra else None
>    +               comment: Optional[str] = None):
>         extra = Extra(comment, ifcond)
>         if features:
>             obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]
>    @@ -170,16 +169,16 @@ const QLitObject %(c_name)s = %(c_string)s;
>             return self._name(typ.name)
>     
>         def _gen_tree(self, name, mtype, obj, ifcond, features):
>    -        extra = None
>    +        comment = None
>             if mtype not in ('command', 'event', 'builtin', 'array'):
>                 if not self._unmask:
>                     # Output a comment to make it easy to map masked names
>                     # back to the source when reading the generated output.
>    -                extra = Extra(comment=f'"{self._name(name)}" = {name}')
>    +                comment = f'"{self._name(name)}" = {name}'
>                 name = self._name(name)
>             obj['name'] = name
>             obj['meta-type'] = mtype
>    -        self._trees.append(_make_tree(obj, ifcond, features, extra))
>    +        self._trees.append(_make_tree(obj, ifcond, features, comment))
>     
>         def _gen_member(self, member):
>             obj = {'name': member.name, 'type': self._use_type(member.type)}
> 
> I understand you're trying to just make minimal refactoring, and I
> don't think this should block your cleanup series.  So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>

I appreciate the benefit-of-the-doubt, but I think this change is worth 
making while we're here.

> 
>>       if features:
>> -        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
>> -    if extra:
>> -        return (obj, extra)
>> -    return obj
>> +        obj['features'] = [(f.name, Extra(None, f.ifcond)) for f in features]
>> +    return (obj, extra)
>>   
>>   
>>   def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
>> @@ -40,8 +47,8 @@ def indent(level):
>>   
>>       if isinstance(obj, tuple):
>>           ifobj, extra = obj
>> -        ifcond = extra.get('if')
>> -        comment = extra.get('comment')
>> +        ifcond = extra.ifcond
>> +        comment = extra.comment
>>           ret = ''
>>           if comment:
>>               ret += indent(level) + '/* %s */\n' % comment
>> @@ -168,7 +175,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>               if not self._unmask:
>>                   # Output a comment to make it easy to map masked names
>>                   # back to the source when reading the generated output.
>> -                extra = {'comment': '"%s" = %s' % (self._name(name), name)}
>> +                extra = Extra(comment=f'"{self._name(name)}" = {name}')
>>               name = self._name(name)
>>           obj['name'] = name
>>           obj['meta-type'] = mtype
>> -- 
>> 2.26.2
>>
>