John Snow <js...@redhat.com> writes:
> On 2/24/21 5:39 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@redhat.com>
>>> Reviewed-by: Eduardo Habkost <ehabk...@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.
