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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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 <address@hidden>
>> ---
>>  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?

Accident.

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

I'll drop them,

>>      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 admit I wobble between

    raise QAPISemError(info, "some lengthy error message text with %s"
                       % argument)

and

    raise QAPISemError(info,
                       "some lengthy error message text with %s" % argument)

and

    raise QAPISemError(
        info, "some lengthy error message text with %s" % argument)

The first looks okay, but as a rule, lines should be broken at an
operator with lowest precedence.

The second tends to produce long lines.

I'm still getting used to the third.

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

I'll have another look.

Thanks!