Message ID | 20210223003408.964543-7-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
Series | qapi: static typing conversion, pt3 | expand |
John Snow <jsnow@redhat.com> writes: > Iterating over the members of data isn't going to work if it's not some > form of dict anyway, but for the sake of mypy, formalize it. > > Signed-off-by: John Snow <jsnow@redhat.com> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > scripts/qapi/expr.py | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index c97e8ce8a4d..afa6bd07769 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -254,6 +254,9 @@ def check_union(expr, info): > raise QAPISemError(info, "'discriminator' requires 'base'") > check_name_is_str(discriminator, info, "'discriminator'") > > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_str(key, info, source) > @@ -267,6 +270,10 @@ def check_alternate(expr, info): > > if not members: > raise QAPISemError(info, "'data' must not be empty") > + > + if not isinstance(members, dict): > + raise QAPISemError(info, "'data' must be an object") > + > for (key, value) in members.items(): > source = "'data' member '%s'" % key > check_name_str(key, info, source) All errors require a negative test. If an error is unreachable, it should be an assertion instead. If these new errors are reachable, the commit might be a bug fix.
On 2/24/21 5:39 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> Iterating over the members of data isn't going to work if it's not some >> form of dict anyway, but for the sake of mypy, formalize it. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >> --- >> scripts/qapi/expr.py | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index c97e8ce8a4d..afa6bd07769 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -254,6 +254,9 @@ def check_union(expr, info): >> raise QAPISemError(info, "'discriminator' requires 'base'") >> check_name_is_str(discriminator, info, "'discriminator'") >> >> + if not isinstance(members, dict): >> + raise QAPISemError(info, "'data' must be an object") >> + >> for (key, value) in members.items(): >> source = "'data' member '%s'" % key >> check_name_str(key, info, source) >> @@ -267,6 +270,10 @@ def check_alternate(expr, info): >> >> if not members: >> raise QAPISemError(info, "'data' must not be empty") >> + >> + if not isinstance(members, dict): >> + raise QAPISemError(info, "'data' must be an object") >> + >> for (key, value) in members.items(): >> source = "'data' member '%s'" % key >> check_name_str(key, info, source) > > All errors require a negative test. > > If an error is unreachable, it should be an assertion instead. > > If these new errors are reachable, the commit might be a bug fix. > You can, yes: Traceback (most recent call last): File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module> sys.exit(main.main()) File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 89, in main generate(args.schema, File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 51, in generate schema = QAPISchema(schema_file) File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 853, in __init__ exprs = check_exprs(parser.exprs) File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 337, in check_exprs check_union(expr, info) File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 248, in check_union for (key, value) in members.items(): AttributeError: 'list' object has no attribute 'items' from this beauty: ## # @Foo: # # @id: identifier of the backend # # @driver: the backend driver to use # # @timer-period: timer period (in microseconds, 0: use lowest possible) # # Since: 4.0 ## { 'union': 'Foo', 'base': { 'id': 'str', 'driver': 'AudiodevDriver', '*timer-period': 'uint32' }, 'discriminator': 'driver', 'data': ['hello', 'world'] } I'll add some new regression tests for you. Do you want them squished in with this commit, or would you like it done kind of independently, at the beginning of this series instead? --js
John Snow <jsnow@redhat.com> writes: > On 2/24/21 5:39 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> Iterating over the members of data isn't going to work if it's not some >>> form of dict anyway, but for the sake of mypy, formalize it. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> scripts/qapi/expr.py | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >>> index c97e8ce8a4d..afa6bd07769 100644 >>> --- a/scripts/qapi/expr.py >>> +++ b/scripts/qapi/expr.py >>> @@ -254,6 +254,9 @@ def check_union(expr, info): >>> raise QAPISemError(info, "'discriminator' requires 'base'") >>> check_name_is_str(discriminator, info, "'discriminator'") >>> >>> + if not isinstance(members, dict): >>> + raise QAPISemError(info, "'data' must be an object") >>> + >>> for (key, value) in members.items(): >>> source = "'data' member '%s'" % key >>> check_name_str(key, info, source) >>> @@ -267,6 +270,10 @@ def check_alternate(expr, info): >>> >>> if not members: >>> raise QAPISemError(info, "'data' must not be empty") >>> + >>> + if not isinstance(members, dict): >>> + raise QAPISemError(info, "'data' must be an object") >>> + >>> for (key, value) in members.items(): >>> source = "'data' member '%s'" % key >>> check_name_str(key, info, source) >> >> All errors require a negative test. >> >> If an error is unreachable, it should be an assertion instead. >> >> If these new errors are reachable, the commit might be a bug fix. >> > > You can, yes: > > Traceback (most recent call last): > File "/home/jsnow/src/qemu/scripts/qapi-gen.py", line 19, in <module> > sys.exit(main.main()) > File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 89, in main > generate(args.schema, > File "/home/jsnow/src/qemu/scripts/qapi/main.py", line 51, in generate > schema = QAPISchema(schema_file) > File "/home/jsnow/src/qemu/scripts/qapi/schema.py", line 853, in __init__ > exprs = check_exprs(parser.exprs) > File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 337, in > check_exprs > check_union(expr, info) > File "/home/jsnow/src/qemu/scripts/qapi/expr.py", line 248, in > check_union > for (key, value) in members.items(): > AttributeError: 'list' object has no attribute 'items' > > > from this beauty: > > ## > # @Foo: > # > # @id: identifier of the backend > # > # @driver: the backend driver to use > # > # @timer-period: timer period (in microseconds, 0: use lowest possible) > # > # Since: 4.0 > ## > { 'union': 'Foo', > 'base': { > 'id': 'str', > 'driver': 'AudiodevDriver', > '*timer-period': 'uint32' }, > 'discriminator': 'driver', > 'data': ['hello', 'world'] > } Definitely a bug fix. The commit message should say so. It's not just for mypy's sake. > I'll add some new regression tests for you. Do you want them squished in > with this commit, or would you like it done kind of independently, at > the beginning of this series instead? I prefer the latter, because then bug fix patch nicely illustrates what's being fixed. Preference, not demand.
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index c97e8ce8a4d..afa6bd07769 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -254,6 +254,9 @@ def check_union(expr, info): raise QAPISemError(info, "'discriminator' requires 'base'") check_name_is_str(discriminator, info, "'discriminator'") + if not isinstance(members, dict): + raise QAPISemError(info, "'data' must be an object") + for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_str(key, info, source) @@ -267,6 +270,10 @@ def check_alternate(expr, info): if not members: raise QAPISemError(info, "'data' must not be empty") + + if not isinstance(members, dict): + raise QAPISemError(info, "'data' must be an object") + for (key, value) in members.items(): source = "'data' member '%s'" % key check_name_str(key, info, source)