Re: [PATCH 06/25] qapi: Change frontend error messages to start with lower case

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3566 bytes --]

On 9/24/19 8:28 AM, Markus Armbruster wrote:
> Starting error messages with a capital letter complicates things when
> text can get interpolated both at the beginning and in the middle of
> an error message.  The next patch will do that.  Switch to lower case
> to keep it simpler.
> 
> For what it's worth, the GNU Coding Standards advise the message
> "should not begin with a capital letter when it follows a program name
> and/or file name, because that isn’t the beginning of a sentence. (The
> sentence conceptually starts at the beginning of the line.)"

We're inconsistent throughout the code base, but this is one place where
I like the GCS rationale.  Fixing it everywhere may not be worth the
churn, but fixing it within the subset of the qapi generator is worthwhile.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi/common.py                        | 175 +++++++++---------
>  tests/qapi-schema/alternate-any.err           |   2 +-

>  tests/qapi-schema/unknown-expr-key.err        |   2 +-
>  125 files changed, 215 insertions(+), 204 deletions(-)
>  create mode 100644 tests/qapi-schema/escape-too-big.err
>  create mode 100644 tests/qapi-schema/string-control.err
>  create mode 100644 tests/qapi-schema/string-unclosed.err
>  create mode 100644 tests/qapi-schema/string-unicode.err

Umm, what's going on here?

You'll want to either drop these files (if they were leftovers in your
working directory from previous points in time), or defer their addition
to when the corresponding actual tests exist.

>      def get_doc(self, info):
>          if self.val != '##':
> -            raise QAPIParseError(self, "Junk after '##' at start of "
> -                                 "documentation comment")
> +            raise QAPIParseError(
> +                self, "junk after '##' at start of documentation comment")

Reformatting like this also makes grepping for a particular message easier.


> @@ -868,8 +869,8 @@ def check_union(expr, info):
>          enum_values = members.keys()
>          allow_metas = ['built-in', 'union', 'alternate', 'struct', 'enum']
>          if base is not None:
> -            raise QAPISemError(info, "Simple union '%s' must not have a base" %
> -                               name)
> +            raise QAPISemError(
> +                info, "simple union '%s' must not have a base" % name)
>  

A bit odd that you reformat here to get the second argument all on one
line...

>      # Else, it's a flat union.
>      else:
> @@ -878,46 +879,47 @@ def check_union(expr, info):
>                     base, allow_dict=name,
>                     allow_metas=['struct'])
>          if not base:
> -            raise QAPISemError(info, "Flat union '%s' must have a base"
> +            raise QAPISemError(info, "flat union '%s' must have a base"
>                                 % name)

...but not here.  The reformatting is not the primary focus of the
patch, and doesn't hurt semantically whether or not you do it, but maybe
it is worth calling out in the commit message the criteria you used for
deciding when to reformat, and/or make the patch strive for more
consistency.  I'll leave that up to you; fixing the spurious new files,
and making your choice of where to place the linebreaks, doesn't affect
my ability to offer:

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]