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
>>
>