| Message ID | 20200922210101.4081073-31-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
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. 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 <ehabkost@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 >
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 >> >
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) 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
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(-)