Re: [PATCH 21/25] qapi: Avoid redundant definition references in error messages

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!