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 <js...@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.
If you are going through the trouble of changing the type of the
4rd argument to _make_tree(), this seems more obvious:
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 <ehabk...@redhat.com>
> 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
>
--
Eduardo
