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)
+ check_if(expr, info, meta.value)
check_features(expr.get('features'), info)
check_flags(expr, info)
--
2.26.2
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
>
--
Eduardo
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? > > >
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 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 >
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
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
© 2016 - 2026 Red Hat, Inc.