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 <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.
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 <ehabk...@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
