| Message ID | 20200922211313.4082880-17-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt2 | expand |
On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote: > This enforces a type signature against all of the top-level expression > check routines without necessarily needing to create some > overcomplicated class hierarchy for them. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 69 ++++++++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 34 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 74b2681ef8..cfd342aa04 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -31,8 +31,11 @@ > structures and contextual semantic validation. > """ > > +from enum import Enum > import re > from typing import ( > + Callable, > + Dict, > Iterable, > List, > MutableMapping, > @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None: > check_type(args, info, "'data'", allow_dict=not boxed) > > > +class ExpressionType(str, Enum): > + INCLUDE = 'include' > + ENUM = 'enum' > + UNION = 'union' > + ALTERNATE = 'alternate' > + STRUCT = 'struct' > + COMMAND = 'command' > + EVENT = 'event' > + > + > +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = { > + 'enum': check_enum, > + 'union': check_union, > + 'alternate': check_alternate, > + 'struct': check_struct, > + 'command': check_command, > + 'event': check_event, > +} > + > + > def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > """ > Validate and normalize a list of parsed QAPI schema expressions. [RW] > @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > assert tmp is None or isinstance(tmp, QAPIDoc) > doc: Optional[QAPIDoc] = tmp > > - if 'include' in expr: > - continue > - > - if 'enum' in expr: > - meta = 'enum' > - elif 'union' in expr: > - meta = 'union' > - elif 'alternate' in expr: > - meta = 'alternate' > - elif 'struct' in expr: > - meta = 'struct' > - elif 'command' in expr: > - meta = 'command' > - elif 'event' in expr: > - meta = 'event' > + for kind in ExpressionType: > + if kind in expr: > + meta = kind I see lots of meta.value expressions below. Maybe setting meta = kind.value will make the code more readable? I don't think this should block an obvious improvement to the code, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + break > else: > raise QAPISemError(info, "expression is missing metatype") > > + if meta == ExpressionType.INCLUDE: > + continue > + > name = cast(str, expr[meta]) # asserted right below: > - check_name_is_str(name, info, "'%s'" % meta) > - info.set_defn(meta, name) > - check_defn_name_str(name, info, meta) > + check_name_is_str(name, info, "'%s'" % meta.value) > + info.set_defn(meta.value, name) > + check_defn_name_str(name, info, meta.value) > > if doc: > if doc.symbol != name: > @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > raise QAPISemError(info, > "documentation comment required") > > - if meta == 'enum': > - check_enum(expr, info) > - elif meta == 'union': > - check_union(expr, info) > - elif meta == 'alternate': > - check_alternate(expr, info) > - elif meta == 'struct': > - check_struct(expr, info) > - elif meta == 'command': > - check_command(expr, info) > - elif meta == 'event': > - check_event(expr, info) > - else: > - assert False, 'unexpected meta type' > - > - check_if(expr, info, meta) > + _CHECK_FN[meta](expr, info) > + check_if(expr, info, meta.value) > check_features(expr.get('features'), info) > check_flags(expr, info) > > -- > 2.26.2 >
On 9/23/20 4:36 PM, Eduardo Habkost wrote: > I see lots of meta.value expressions below. Maybe setting > meta = kind.value > will make the code more readable? > I can do that. > I don't think this should block an obvious improvement to the > code, so: > > Reviewed-by: Eduardo Habkost<ehabkost@redhat.com> Thanks! --js
On Tue, Sep 22, 2020 at 05:13:13PM -0400, John Snow wrote: > This enforces a type signature against all of the top-level expression > check routines without necessarily needing to create some > overcomplicated class hierarchy for them. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 69 ++++++++++++++++++++++---------------------- > 1 file changed, 35 insertions(+), 34 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index 74b2681ef8..cfd342aa04 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -31,8 +31,11 @@ > structures and contextual semantic validation. > """ > > +from enum import Enum > import re > from typing import ( > + Callable, > + Dict, > Iterable, > List, > MutableMapping, > @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None: > check_type(args, info, "'data'", allow_dict=not boxed) > > > +class ExpressionType(str, Enum): > + INCLUDE = 'include' > + ENUM = 'enum' > + UNION = 'union' > + ALTERNATE = 'alternate' > + STRUCT = 'struct' > + COMMAND = 'command' > + EVENT = 'event' > + > + > +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = { > + 'enum': check_enum, > + 'union': check_union, > + 'alternate': check_alternate, > + 'struct': check_struct, > + 'command': check_command, > + 'event': check_event, > +} > + > + > def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > """ > Validate and normalize a list of parsed QAPI schema expressions. [RW] > @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > assert tmp is None or isinstance(tmp, QAPIDoc) > doc: Optional[QAPIDoc] = tmp > > - if 'include' in expr: > - continue > - > - if 'enum' in expr: > - meta = 'enum' > - elif 'union' in expr: > - meta = 'union' > - elif 'alternate' in expr: > - meta = 'alternate' > - elif 'struct' in expr: > - meta = 'struct' > - elif 'command' in expr: > - meta = 'command' > - elif 'event' in expr: > - meta = 'event' > + for kind in ExpressionType: > + if kind in expr: > + meta = kind > + break > else: > raise QAPISemError(info, "expression is missing metatype") > > + if meta == ExpressionType.INCLUDE: > + continue > + > name = cast(str, expr[meta]) # asserted right below: > - check_name_is_str(name, info, "'%s'" % meta) > - info.set_defn(meta, name) > - check_defn_name_str(name, info, meta) > + check_name_is_str(name, info, "'%s'" % meta.value) > + info.set_defn(meta.value, name) > + check_defn_name_str(name, info, meta.value) > > if doc: > if doc.symbol != name: > @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: > raise QAPISemError(info, > "documentation comment required") > > - if meta == 'enum': > - check_enum(expr, info) > - elif meta == 'union': > - check_union(expr, info) > - elif meta == 'alternate': > - check_alternate(expr, info) > - elif meta == 'struct': > - check_struct(expr, info) > - elif meta == 'command': > - check_command(expr, info) > - elif meta == 'event': > - check_event(expr, info) > - else: > - assert False, 'unexpected meta type' > - > - check_if(expr, info, meta) > + _CHECK_FN[meta](expr, info) I have to say the style of this line bothers me, but it's just that, style. So, Reviewed-by: Cleber Rosa <crosa@redhat.com>
On 9/24/20 9:18 PM, Cleber Rosa wrote: > I have to say the style of this line bothers me, but it's just that, > style. So, What don't you like?
Hi, I would replace the word variable "kind" by "category". ./helio On Fri, Sep 25, 2020, 03:32 John Snow <jsnow@redhat.com> wrote: > On 9/24/20 9:18 PM, Cleber Rosa wrote: > > I have to say the style of this line bothers me, but it's just that, > > style. So, > > What don't you like? > > > <div dir="auto">Hi,<div dir="auto"><br></div><div dir="auto">I would replace the word variable "kind" by "category".</div><div dir="auto"><br></div><div dir="auto">./helio</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 25, 2020, 03:32 John Snow <<a href="mailto:jsnow@redhat.com">jsnow@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 9/24/20 9:18 PM, Cleber Rosa wrote:<br> > I have to say the style of this line bothers me, but it's just that,<br> > style. So,<br> <br> What don't you like?<br> <br> <br> </blockquote></div>
On 9/25/20 2:03 AM, Helio Loureiro wrote: > Hi, > > I would replace the word variable "kind" by "category". > Hi, welcome to the list! For patch reviews, we try to reply in-line, below the original post. I'm not attached to 'kind', but 'category' is perhaps too broad. Category in this particular domain might refer to the difference between a "Directive" (include, pragma) and a Definition (enum, struct, union, alternate, command, event) (For more information on the QAPI Schema Language that we are parsing and validating here, see docs/devel/qapi-code-gen.txt if you are curious. Ultimately it is a JSON-like format that permits multiple objects per document and allows comments. We use these structures to generate types and command interfaces for our API protocol, QMP.) Ultimately I am using 'kind' for the 'type of expression', but type is an extremely overloaded word when parsing a language in another language! We also use 'meta' nearby for semantically the same thing, but with different typing. Thanks for looking! --js > ./helio > > On Fri, Sep 25, 2020, 03:32 John Snow <jsnow@redhat.com > <mailto:jsnow@redhat.com>> wrote: > > On 9/24/20 9:18 PM, Cleber Rosa wrote: > > I have to say the style of this line bothers me, but it's just that, > > style. So, > > What don't you like? > >
On Thu, Sep 24, 2020 at 09:32:05PM -0400, John Snow wrote: > On 9/24/20 9:18 PM, Cleber Rosa wrote: > > I have to say the style of this line bothers me, but it's just that, > > style. So, > > What don't you like? It's the sum of the "private" + "global dictionary" + "its item being called directly". But don't bother, this is probably the kind of comment that I should omit, as I don't want you to, say, create a wrapper function around the dict, partially defeating the purpose of this patch. - Cleber.
On 9/25/20 12:38 PM, Cleber Rosa wrote: > On Thu, Sep 24, 2020 at 09:32:05PM -0400, John Snow wrote: >> On 9/24/20 9:18 PM, Cleber Rosa wrote: >>> I have to say the style of this line bothers me, but it's just that, >>> style. So, >> >> What don't you like? > > It's the sum of the "private" + "global dictionary" + "its item being > called directly". > > But don't bother, this is probably the kind of comment that I should > omit, as I don't want you to, say, create a wrapper function around > the dict, partially defeating the purpose of this patch. > ACK, just wanted to know what the style nit was. Thanks :) --js
On Fri, Sep 25, 2020, 16:16 John Snow <jsnow@redhat.com> wrote: > On 9/25/20 2:03 AM, Helio Loureiro wrote: > > Hi, > > > > I would replace the word variable "kind" by "category". > > > > Hi, welcome to the list! > Tkz! > For patch reviews, we try to reply in-line, below the original post. > I realized that later. It has been more than 20 years I don't use this formating. But if I intend to join the pack, I need to follow the pack. > > (For more information on the QAPI Schema Language that we are parsing > and validating here, see docs/devel/qapi-code-gen.txt if you are > curious. Ultimately it is a JSON-like format that permits multiple > objects per document and allows comments. We use these structures to > generate types and command interfaces for our API protocol, QMP.) > Based on that I would suggest 'type_ref' instead to match the definitions over there and since word 'type' itself is reserved. > > ./helio > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 25, 2020, 16:16 John Snow <<a href="mailto:jsnow@redhat.com">jsnow@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 9/25/20 2:03 AM, Helio Loureiro wrote:<br> > Hi,<br> > <br> > I would replace the word variable "kind" by "category".<br> > <br> <br> Hi, welcome to the list!<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Tkz!</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> For patch reviews, we try to reply in-line, below the original post.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I realized that later. It has been more than 20 years I don't use this formating. But if I intend to join the pack, I need to follow the pack.</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br></blockquote></div></div><div dir="auto"><br></div><div dir="auto"></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> (For more information on the QAPI Schema Language that we are parsing <br> and validating here, see docs/devel/qapi-code-gen.txt if you are <br> curious. Ultimately it is a JSON-like format that permits multiple <br> objects per document and allows comments. We use these structures to <br> generate types and command interfaces for our API protocol, QMP.)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Based on that I would suggest 'type_ref' instead to match the definitions over there and since word 'type' itself is reserved.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">./helio</div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div>
On 9/26/20 7:31 AM, Helio Loureiro wrote: > > > On Fri, Sep 25, 2020, 16:16 John Snow <jsnow@redhat.com > <mailto:jsnow@redhat.com>> wrote: > > On 9/25/20 2:03 AM, Helio Loureiro wrote: > > Hi, > > > > I would replace the word variable "kind" by "category". > > > > Hi, welcome to the list! > > > Tkz! > > > For patch reviews, we try to reply in-line, below the original post. > > > I realized that later. It has been more than 20 years I don't use this > formating. But if I intend to join the pack, I need to follow the pack. > > > > (For more information on the QAPI Schema Language that we are parsing > and validating here, see docs/devel/qapi-code-gen.txt if you are > curious. Ultimately it is a JSON-like format that permits multiple > objects per document and allows comments. We use these structures to > generate types and command interfaces for our API protocol, QMP.) > > > Based on that I would suggest 'type_ref' instead to match the > definitions over there and since word 'type' itself is reserved. > One of the unsolvable problems in computer science is naming things: "TYPE-REF" also has a specific meaning in QAPI, as it is the name of one of the BNF grammar tokens we use. So I might suggest (if "kind" is too ambiguous), that I might use "statement_type" or "expression_type" if that helps clarify things. --js
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index 74b2681ef8..cfd342aa04 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -31,8 +31,11 @@ structures and contextual semantic validation. """ +from enum import Enum import re from typing import ( + Callable, + Dict, Iterable, List, MutableMapping, @@ -494,6 +497,26 @@ def check_event(expr: Expression, info: QAPISourceInfo) -> None: check_type(args, info, "'data'", allow_dict=not boxed) +class ExpressionType(str, Enum): + INCLUDE = 'include' + ENUM = 'enum' + UNION = 'union' + ALTERNATE = 'alternate' + STRUCT = 'struct' + COMMAND = 'command' + EVENT = 'event' + + +_CHECK_FN: Dict[str, Callable[[Expression, QAPISourceInfo], None]] = { + 'enum': check_enum, + 'union': check_union, + 'alternate': check_alternate, + 'struct': check_struct, + 'command': check_command, + 'event': check_event, +} + + def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: """ Validate and normalize a list of parsed QAPI schema expressions. [RW] @@ -519,28 +542,20 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: assert tmp is None or isinstance(tmp, QAPIDoc) doc: Optional[QAPIDoc] = tmp - if 'include' in expr: - continue - - if 'enum' in expr: - meta = 'enum' - elif 'union' in expr: - meta = 'union' - elif 'alternate' in expr: - meta = 'alternate' - elif 'struct' in expr: - meta = 'struct' - elif 'command' in expr: - meta = 'command' - elif 'event' in expr: - meta = 'event' + for kind in ExpressionType: + if kind in expr: + meta = kind + break else: raise QAPISemError(info, "expression is missing metatype") + if meta == ExpressionType.INCLUDE: + continue + name = cast(str, expr[meta]) # asserted right below: - check_name_is_str(name, info, "'%s'" % meta) - info.set_defn(meta, name) - check_defn_name_str(name, info, meta) + check_name_is_str(name, info, "'%s'" % meta.value) + info.set_defn(meta.value, name) + check_defn_name_str(name, info, meta.value) if doc: if doc.symbol != name: @@ -551,22 +566,8 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]: raise QAPISemError(info, "documentation comment required") - if meta == 'enum': - check_enum(expr, info) - elif meta == 'union': - check_union(expr, info) - elif meta == 'alternate': - check_alternate(expr, info) - elif meta == 'struct': - check_struct(expr, info) - elif meta == 'command': - check_command(expr, info) - elif meta == 'event': - check_event(expr, info) - else: - assert False, 'unexpected meta type' - - check_if(expr, info, meta) + _CHECK_FN[meta](expr, info) + check_if(expr, info, meta.value) check_features(expr.get('features'), info) check_flags(expr, info)
This enforces a type signature against all of the top-level expression check routines without necessarily needing to create some overcomplicated class hierarchy for them. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 69 ++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 34 deletions(-)