On Wed, Feb 8, 2023, 11:17 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes: > > > This is an immutable, named, typed tuple. It's arguably nicer than > > arbitrary dicts for passing data around when using strict typing. > > > > This patch turns parser.exprs into a list of ParsedExpressions instead, > > and adjusts expr.py to match. > > > > This allows the types we specify in parser.py to be "remembered" all the > > way through expr.py and into schema.py. Several assertions around > > packing and unpacking this data can be removed as a result. > > > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > scripts/qapi/expr.py | 29 +++++++++-------------------- > > scripts/qapi/parser.py | 29 ++++++++++++++++++----------- > > scripts/qapi/schema.py | 6 +++--- > > 3 files changed, 30 insertions(+), 34 deletions(-) > > > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > > index d01543006d8..293f830fe9d 100644 > > --- a/scripts/qapi/expr.py > > +++ b/scripts/qapi/expr.py > > @@ -44,7 +44,7 @@ > > > > from .common import c_name > > from .error import QAPISemError > > -from .parser import QAPIDoc > > +from .parser import ParsedExpression > > from .source import QAPISourceInfo > > > > > > @@ -595,29 +595,17 @@ def check_event(expr: _JSONObject, info: > QAPISourceInfo) -> None: > > check_type(args, info, "'data'", allow_dict=not boxed) > > > > > > -def check_expr(expr_elem: _JSONObject) -> None: > > +def check_expr(pexpr: ParsedExpression) -> None: > > """ > > - Validate and normalize a parsed QAPI schema expression. > > + Validate and normalize a `ParsedExpression`. > > Doc comment style question: what's our attitude towards repeating (parts > of) the function signature in its doc comment? > > In general, I dislike comments that merely rephrase code as prose. > Me too, I've never found a doc system that gets it exactly right... I don't like the duplication. For the sake of tooltips and help docs that might be removed from code, I often feel compelled to write something. Sometimes I value the consistency over the DRY violation. I dunno. I struggle with it. > Untyped Python builds a habit of mentioning the types in the doc > comment. That's additional information. In typed Python, it's > rephrased information. > > Keeping the old line would be just fine with me. > The only reason I have this habit is because I have an obsession with cross-references, really. I love clickables. love 'em! Though it's probably true that the type signature would suffice for giving you a clickable. Looking at https://qemu.readthedocs.io/projects/python-qemu-qmp/en/latest/overview.html ... yeah, the signatures are clickable. OK, I can cut it out. probably. I guess. perhaps. > > > > - :param expr_elem: The parsed expression to normalize and validate. > > + :param pexpr: The parsed expression to normalize and validate. > > > > :raise QAPISemError: When this expression fails validation. > > - :return: None, ``expr`` is normalized in-place as needed. > > + :return: None, ``pexpr`` is normalized in-place as needed. > > """ > > - # Expression > > - assert isinstance(expr_elem['expr'], dict) > > - for key in expr_elem['expr'].keys(): > > - assert isinstance(key, str) > > - expr: _JSONObject = expr_elem['expr'] > > - > > - # QAPISourceInfo > > - assert isinstance(expr_elem['info'], QAPISourceInfo) > > - info: QAPISourceInfo = expr_elem['info'] > > - > > - # Optional[QAPIDoc] > > - tmp = expr_elem.get('doc') > > - assert tmp is None or isinstance(tmp, QAPIDoc) > > - doc: Optional[QAPIDoc] = tmp > > + expr = pexpr.expr > > + info = pexpr.info > > > > if 'include' in expr: > > return > > @@ -637,6 +625,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > > info.set_defn(meta, name) > > check_defn_name_str(name, info, meta) > > > > + doc = pexpr.doc > > if doc: > > if doc.symbol != name: > > raise QAPISemError( > > @@ -688,7 +677,7 @@ def check_expr(expr_elem: _JSONObject) -> None: > > check_flags(expr, info) > > > > > > -def check_exprs(exprs: List[_JSONObject]) -> List[_JSONObject]: > > +def check_exprs(exprs: List[ParsedExpression]) -> > List[ParsedExpression]: > > """ > > Validate and normalize a list of parsed QAPI schema expressions. > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index 1b006cdc133..f897dffbfd4 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -21,6 +21,7 @@ > > TYPE_CHECKING, > > Dict, > > List, > > + NamedTuple, > > Optional, > > Set, > > Union, > > @@ -48,6 +49,12 @@ > > # several modules. > > > > > > +class ParsedExpression(NamedTuple): > > + expr: TopLevelExpr > > + info: QAPISourceInfo > > + doc: Optional['QAPIDoc'] > > + > > + > > class QAPIParseError(QAPISourceError): > > """Error class for all QAPI schema parsing errors.""" > > def __init__(self, parser: 'QAPISchemaParser', msg: str): > > @@ -100,7 +107,7 @@ def __init__(self, > > self.line_pos = 0 > > > > # Parser output: > > - self.exprs: List[Dict[str, object]] = [] > > + self.exprs: List[ParsedExpression] = [] > > self.docs: List[QAPIDoc] = [] > > > > # Showtime! > > @@ -147,8 +154,7 @@ def _parse(self) -> None: > > "value of 'include' must be a > string") > > incl_fname = os.path.join(os.path.dirname(self._fname), > > include) > > - self.exprs.append({'expr': {'include': incl_fname}, > > - 'info': info}) > > + self._add_expr(OrderedDict({'include': incl_fname}), > info) > > exprs_include = self._include(include, info, incl_fname, > > self._included) > > if exprs_include: > > @@ -165,17 +171,18 @@ def _parse(self) -> None: > > for name, value in pragma.items(): > > self._pragma(name, value, info) > > else: > > - expr_elem = {'expr': expr, > > - 'info': info} > > - if cur_doc: > > - if not cur_doc.symbol: > > - raise QAPISemError( > > - cur_doc.info, "definition documentation > required") > > - expr_elem['doc'] = cur_doc > > - self.exprs.append(expr_elem) > > + if cur_doc and not cur_doc.symbol: > > + raise QAPISemError( > > + cur_doc.info, "definition documentation > required") > > + self._add_expr(expr, info, cur_doc) > > cur_doc = None > > self.reject_expr_doc(cur_doc) > > > > + def _add_expr(self, expr: TopLevelExpr, > > + info: QAPISourceInfo, > > + doc: Optional['QAPIDoc'] = None) -> None: > > + self.exprs.append(ParsedExpression(expr, info, doc)) > > + > > @staticmethod > > def reject_expr_doc(doc: Optional['QAPIDoc']) -> None: > > if doc and doc.symbol: > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index cd8661125cd..89f8e60db36 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -1168,9 +1168,9 @@ def _def_event(self, expr, info, doc): > > > > def _def_exprs(self, exprs): > > for expr_elem in exprs: > > Rename @expr_elem to @pexpr for consistency with check_expr()? > OK, will address when reordering/squashing > > - expr = expr_elem['expr'] > > - info = expr_elem['info'] > > - doc = expr_elem.get('doc') > > + expr = expr_elem.expr > > + info = expr_elem.info > > + doc = expr_elem.doc > > if 'enum' in expr: > > self._def_enum_type(expr, info, doc) > > elif 'struct' in expr: > >
