Eric Blake <ebl...@redhat.com> writes:
> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> Many error messages refer to the offending definition even though
>> they're preceded by an "in definition" line. Rephrase them.
>
> This is the cleanup promised earlier in the series.
>
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> scripts/qapi/common.py | 113 +++++++-----------
>> tests/qapi-schema/alternate-array.err | 2 +-
>
>> def check_command(expr, info):
>> - name = expr['command']
>> args = expr.get('data')
>> boxed = expr.get('boxed', False)
>>
>> if boxed and args is None:
>> raise QAPISemError(info, "'boxed': true requires 'data'")
>> - check_type(args, info, "'data' for command '%s'" % name,
>> - allow_dict=not boxed)
>> - check_type(expr.get('returns'), info,
>> - "'returns' for command '%s'" % name,
>> - allow_array=True)
>> + check_type(expr.get('data'), info, "'data'", allow_dict=not boxed)
>> + check_type(expr.get('returns'), info, "'returns'", allow_array=True)
>
> Why are you repeating expr.get('dat') here instead of reusing args? I
> guess it adds consistency with the expr.get('returns') in the next line.
Don't remember, might have been an accident. If I want consistency, I
guess I should add rets = expr.get('returns').
>>
>>
>> def check_event(expr, info):
>> - name = expr['event']
>> args = expr.get('data')
>> boxed = expr.get('boxed', False)
>>
>> if boxed and args is None:
>> raise QAPISemError(info, "'boxed': true requires 'data'")
>> - check_type(args, info, "'data' for event '%s'" % name,
>> - allow_dict=not boxed)
>> + check_type(expr.get('data'), info, "'data'", allow_dict=not boxed)
> Again, why not reuse args?
>
>
>> +++ b/tests/qapi-schema/args-member-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/args-member-case.json: In command
>> 'no-way-this-will-get-whitelisted':
>> -tests/qapi-schema/args-member-case.json:2: member of 'data' for command
>> 'no-way-this-will-get-whitelisted' uses uppercase in name 'Arg'
>> +tests/qapi-schema/args-member-case.json:2: 'data' member 'Arg' uses
>> uppercase in name 'Arg'
>
> Better, but still feels redundant for calling out 'Arg' twice. Maybe
> you further clean this one later?
check_type()'s error messages interpolate @source received from by
caller. Need to review all its messages and callers.
>> +++ b/tests/qapi-schema/enum-member-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/enum-member-case.json: In enum
>> 'NoWayThisWillGetWhitelisted':
>> -tests/qapi-schema/enum-member-case.json:4: member of enum
>> 'NoWayThisWillGetWhitelisted' uses uppercase in name 'Value'
>> +tests/qapi-schema/enum-member-case.json:4: 'data' member uses uppercase in
>> name 'Value'
>
> Here's a similar error about uppercase that does not have the
> redundancy, for comparison.
>
>
>> +++ b/tests/qapi-schema/union-branch-case.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/union-branch-case.json: In union 'Uni':
>> -tests/qapi-schema/union-branch-case.json:2: member of union 'Uni' uses
>> uppercase in name 'Branch'
>> +tests/qapi-schema/union-branch-case.json:2: 'data' member 'Branch' uses
>> uppercase in name 'Branch'
>
> Another related one.
>
>> +++ b/tests/qapi-schema/union-optional-branch.err
>> @@ -1,2 +1,2 @@
>> tests/qapi-schema/union-optional-branch.json: In union 'Union':
>> -tests/qapi-schema/union-optional-branch.json:2: member of union 'Union'
>> uses invalid name '*a'
>> +tests/qapi-schema/union-optional-branch.json:2: 'data' member '*a' uses
>> invalid name '*a'
>
> Similar type of redundancy, but this time not related to uppercase.
Same problem, different function: check_name_str().
> Overall an improvement.
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
