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

[Eric Blake]

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.

>  
>  
>  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?

> +++ 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.

Overall an improvement.

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature